Bug 215792

If it happens only on pipewire, it might come from the pipewire's specific way to configure the PCM parameters.

The recent relevant change was the restriction of available parameters for the implicit feedback mode and the shared clock source.  In both changes, the secondary opened stream is restricted only to the parameters that are aligned with the first one.  The sample rate, the channels or whatever.

If that's the case, the fact it worked before was a casual case that shouldn't have been supported but was just ignored.

So, for further debugging, you need to show which exact parameters are being set up for which streams in which timing.

Created attachment 300681 [details]
Jack is loaded - stream0 of my Scarlett

Created attachment 300682 [details]
Jack is loaded - capture sub0 SW/HW params

Created attachment 300683 [details]
Pipewire is loaded - stream 0 of my card

Created attachment 300684 [details]
Pipewire is loaded - capture sub0 SW/HW params

Created attachment 300685 [details]
pw-dump output

It contains detail about how pipewire try to configure the alsa device.

From the line 191 of the pipewire state dump (attachment 300685 [details] ) There is the Scarlett 18i8 config with the error : "Start error: Invalid argument"

@Takashi Iwai
About the "timing" to set the parameter, I believe you mean what's the chronology to set parameter to the alsa device ? 
If it's the case, I can't answer easily by myself unless you have a trick for that. On this case, I would ask the Pipewire developer to answer your request.

(In reply to michael from comment #7)
> From the line 191 of the pipewire state dump (attachment 300685 [details] )
> There is the Scarlett 18i8 config with the error : "Start error: Invalid
> argument"

Sorry it's at the line 5222 to retrieve how Pipewire try to configure Alsa.

Well, you need to show what function call failed.  It shows only EINVAL error and it alone doesn't mean anything useful.  If pw shows which ALSA-library call failed with which parameter, it would be more understanding.

As you see in comment 4, the playback stream is already set up for interface 1 altset 1, which is bound with the capture stream interface 2 altset 1.
And this capture setup (interface 2 altset 1) is restricted to channels = 20, and you must open all those channels no matter how many you need out of them.

> Well, you need to show what function call failed.

It is in both cases this function call:

        CHECK(snd_pcm_hw_params_any(hndl, params), "Broken configuration: no configurations available");

And

        CHECK(snd_pcm_hw_params_any(hndl, params), "Broken configuration for playback: no configurations available");

https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/spa/plugins/alsa/alsa-pcm.c#L1363

We call this before we find a compatible format to set.

Interesting.  It means that setting the playback stream restricts too much on the capture side.

We need to know exactly which parameters are set up to the playback stream instead.  This defines the restriction.  My wild guess is that it brought too big capture period size or unaligned size due to the difference of the channels in both playback and capture streams.  Often using 48000 Hz helps in such a case.

Also, in sound/usb/pcm.c, add '#define HW_CONST_DEBUG 1' at the beginning of the file, and rebuild the kernel module.  Then boot with snd_usb_audio.dyndbg=+p boot option.  This will enable the debug prints.  And you'll see a bit more detailed debug messages around the hw_params problems.

Created attachment 300708 [details]
output of kernel messages using aplay followed by arecord on scarlett 18i8

Comment on attachment 300708 [details]
output of kernel messages using aplay followed by arecord on scarlett 18i8

I can reproduce this issue using aplay followed by arecord
If I invoke both commands in reverse order everything works as expected.

Created attachment 300709 [details]
output of kernel messages using arecord followed by aplay

Launching arecord first works fine and both streams coexist withtout issues

Could you give the parameters (found in proc files, at least /proc/asound/card*/pcm*/sub*/hw_params and /proc/asound/card*/stream* files) in both working and non-working cases?  That's the missing information.

And, build the module with with '#define HW_CONST_DEBUG 1' at the beginning of sound/usb/pcm.c.  This will give more verbose outputs.

Last but not least, please test both in 44.1k and 48k.  The behavior may differ for both.

Created attachment 300710 [details]
Output of /proc/asound for working and non working case at 48khz

Hi Takashi,

here's the output or 48khz for both cases, will try out 44.1khz now
Kernel compiling will have to wait until the weekend/next week unfortunately

Created attachment 300711 [details]
Output of /proc/asound for working and non working case at 44.1khz

Here you go.
do you also need the output of the kernel debug messages?

Thanks.  Could you give the hw_params proc also from the capture stream (while it's working), too?

And yes, the rebuilt kernel with the define will reveal why the hw_params is not accepted.  You'd need to rebuild the kernel in anyway when a fix patch is provided :)

(In reply to Takashi Iwai from comment #19)
> Thanks.  Could you give the hw_params proc also from the capture stream
> (while it's working), too?

Never mind, I see that the output has both, just concatenate.

Exactly, sorry if the output was confusing.

The problem with rebuilding the kernel is just time, I have never added a custom kernel to my distribution and don't have  VM ready for testing.
All that said, I'll give it a shot later tonight.
What extra logs are you interested in? Kernel logs when opening the capture card fails?

The likely cause is the buffer size.  In the working case, it reaches to 1MB for the capture stream, which is the hard limit defined in the driver.
OTOH, in the non-working case, the playback buffer (that has only 8 channels) assign a large buffer size.  Since the period size is tied in both playback and capture streams, the minimum buffer size of the capture stream would exceed over 1MB limit, hence the driver returns as -EINVAL.

So, the easiest workaround would be to increase the 1MB limit in the driver, simply put, the following oneliner:

--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -669,7 +669,7 @@ static const struct snd_pcm_hardware snd_usb_hardware =
                                SNDRV_PCM_INFO_PAUSE,
        .channels_min =         1,
        .channels_max =         256,
-       .buffer_bytes_max =     1024 * 1024,
+       .buffer_bytes_max =     4 * 1024 * 1024,
        .period_bytes_min =     64,
        .period_bytes_max =     512 * 1024,
        .periods_min =          2,

... though, the above change might still not work, if the application tries to allocate even bigger buffer because of the available size.  Let's see.

We need the additional hw_params constraint to limit the size for the counter-part stream if it's tied with implicit fb.

(In reply to Takashi Iwai from comment #22)
> The likely cause is the buffer size.  In the working case, it reaches to 1MB
> for the capture stream, which is the hard limit defined in the driver.
> OTOH, in the non-working case, the playback buffer (that has only 8
> channels) assign a large buffer size.  Since the period size is tied in both
> playback and capture streams, the minimum buffer size of the capture stream
> would exceed over 1MB limit, hence the driver returns as -EINVAL.
> 
> So, the easiest workaround would be to increase the 1MB limit in the driver,
> simply put, the following oneliner:
> 
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -669,7 +669,7 @@ static const struct snd_pcm_hardware snd_usb_hardware =
>                                 SNDRV_PCM_INFO_PAUSE,
>         .channels_min =         1,
>         .channels_max =         256,
> -       .buffer_bytes_max =     1024 * 1024,
> +       .buffer_bytes_max =     4 * 1024 * 1024,
>         .period_bytes_min =     64,
>         .period_bytes_max =     512 * 1024,
>         .periods_min =          2,

built a custom kernel with this patch and it does fix the issue.
I can build another one with the debug logging if you think it's necessary.
thanks for the support in this!

Created attachment 300712 [details]
Output of /proc/asound with propsed patch applied

Here's the output of
cat /proc/asound/USB/pcm*/sub*/hw_params && echo '' && cat /proc/asound/USB/stream*
after doing
aplay -D hw:USB -r 48000 -f S32_LE -c 8 /dev/zero
arecord -D hw:USB -r 48000 -f S32_LE -c 20 out.wav

I pushed a PipeWire patch that limits the requested buffer_frames to (with default settings) 8192 * 4 samples. That should be 8192 * 4 * 4 * 8 = 1048576 bytes for 8 channels, 32 bit samples. That's probably better than asking for as much as possible.

(In reply to Wim Taymans from comment #26)
> I pushed a PipeWire patch that limits the requested buffer_frames to (with
> default settings) 8192 * 4 samples. That should be 8192 * 4 * 4 * 8 =
> 1048576 bytes for 8 channels, 32 bit samples. That's probably better than
> asking for as much as possible.

Thanks, that sounds better, at least for the operation like pipewire.

Since my guess seemed correct, below is a more comprehensive fix patch.
Claudio, could you revert the buffer byte change, and test this patch instead?

Created attachment 300713 [details]
Test fix patch

If I am correct then this patch does not exceed the 1MB buffer limit, right?

I am not convinced, if it's good for all cases, because simple calculation for 10 channels (GoXLR for example) with 3 bytes per sample ends with:

   (1024*1024) / (10 * 3) = 34952  .... frames

If you take in account the sampling frequency 192kHz, it's buffer only for 182ms.

We should allow a bigger buffers for this hw (multichannel & 24-bit).

(In reply to Takashi Iwai from comment #28)
> Since my guess seemed correct, below is a more comprehensive fix patch.
> Claudio, could you revert the buffer byte change, and test this patch
> instead?

The new patch works nicely for the Scarlett 18i8.
Let me know if there's anything else you need me to do or test.

(In reply to Jaroslav Kysela from comment #30)
> If I am correct then this patch does not exceed the 1MB buffer limit, right?

Yes but only if the streams are tied in the implicit feedback mode.

> I am not convinced, if it's good for all cases, because simple calculation
> for 10 channels (GoXLR for example) with 3 bytes per sample ends with:
> 
>    (1024*1024) / (10 * 3) = 34952  .... frames
> 
> If you take in account the sampling frequency 192kHz, it's buffer only for
> 182ms.
> 
> We should allow a bigger buffers for this hw (multichannel & 24-bit).

The current definitions of the max buffer and period sizes are just "some" software limit, which we thought it'd be large enough in 20 years ago :)
Surely it's time to increase the sizes.  But it's basically a different issue.
Even if we increase the sizes, the same problem would hit again unless we add the correction of the buffer size reduction by channels.

(Note that the increase of buffer_bytes_max worked around this case because we increased only the buffer size, and not the period size.)

The fix patch submitted to upsteram:
  https://lore.kernel.org/r/20220407211657.15087-1-tiwai@suse.de

The increase of the max buffer size was also submitted:
  https://lore.kernel.org/r/20220407212740.17920-1-tiwai@suse.de

Both will be merged sooner or later, and will be included in 5.18-rc3 pull request.

Meanwhile, it might be cleaner to rather have restrictions via PERIOD_TIME or BUFFER_TIME and drop the limit of bytes instead.

Could you try the patch below instead of the previous fixes (also drop the increase of buffer_bytes_max) and check whether it also works around the problem with your device?

Created attachment 300738 [details]
Alternative fix patch (to use max time instead of bytes)

I just tried this path and it works flawlessly for me ! https://bugzilla.kernel.org/attachment.cgi?id=300713&action=diff

The fixes went into 5.18-rc3 and backported to stable trees.