Re: [PATCH 1/2] ASoC: intel: atom: Stop advertising non working S24LE support

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 3/11/21 10:44 AM, Hans de Goede wrote:
> Hi,
> 
> On 3/9/21 4:42 PM, Pierre-Louis Bossart wrote:
>>
>>
>> On 3/9/21 4:55 AM, Hans de Goede wrote:
>>> The SST firmware's media and deep-buffer inputs are hardcoded to
>>> S16LE, the corresponding DAIs don't have a hw_params callback and
>>> their prepare callback also does not take the format into account.
>>>
>>> So far the advertising of non working S24LE support has not caused
>>> issues because pulseaudio defaults to S16LE, but changing pulse-audio's
>>> config to use S24LE will result in broken sound.
>>>
>>> Pipewire is replacing pulse now and pipewire prefers S24LE over S16LE
>>> when available, causing the problem of the broken S24LE support to
>>> come to the surface now.
>>>
>>> Cc: stable@vger.kernel.org
>>> BugLink: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/866
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Humm, that is strange.
>> I can't recall such limitations in the firmware, and the SSP support does make use of 24 bits.
>> Please give me a couple of days to double-check what's missing.
> 
> Note this is not about the format between the DSP (the DSP's SSP) and the codec,
> this is the format between userspace and the DSP.
> 
> As is mentioned by the reporter of this issue:
> https://github.com/thesofproject/sof/issues/3868#issuecomment-796809535
> Both in that issue but also here:
> https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/530#note_791736
> 
> And independently reproduced by my here:
> https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/866#note_830336
> 
> The S24LE format ATM does not work when passed from userspace, this is
> supposed to take 24 bits sampled packed into 32 bits ints (so padded
> with 1 0 byte to make 32 bits per sample), but to actually get working
> playback with the SST driver, the following commands are necessary:
> 
> ffmpeg -i /usr/share/sounds/alsa/Side_Left.wav -ar 96000 -f s32le -ac 2 test.raw
> aplay --dump-hw-params -D"hw:1,0" -r48000 -c2 -fS24_LE test.raw
> 
> Note how the ffmpeg command to generate a working set of raw samples
> is set to convert to full 32 bit samples, rather then 0 padded 24 bit
> samples. Generating a .raw file with the same -f s32le argument to
> ffmpeg and then playing it with aplay -fS24_LE while using the SOF
> driver results in static. Where as with the SST driver it results
> in working sound. This shows that the 2 clearly interpret the format
> differently and it looks like the SST driver is interpreting it wrong.
> 
> Maybe the SST driver should advertise S32_LE support instead, SOF
> advertised both S24_LE and S32_LE and the S32_LE format is the
> one which works with .raw files generated with ffmpeg's -f s32le
> option when using the SOF drv.
> 
> Note the format is not the only issue though, to get normal speed / pitch
> playback, the file needs to be converted to a sample rate of 96KHz
> and then played back at 48 KHz, hence the "-ar 96000" argument to
> ffmpeg to get normal playback when using aplay -fS24_LE with the SST driver.
> 
> Because of both these fmt and playback speed issues I decided to just
> drop the SNDRV_PCM_FMTBIT_S24_LE support in my patch. I guess we could
> try to fix it, but since the plan is the phase out the SST support for
> these devices this year I believe that we should not spend too much
> time on trying to fix the SST driver here.  Dropping the SNDRV_PCM_FMTBIT_S24_LE
> is a simple workaround to bridge the time until we complete drop the
> SST support.

ok, I agree. I added this capability back in 2015 in the upstream code 
based on the Android versions, and I *think* it was tested, but since 
there's overwhelming evidence of multiple issues let's revert this.

Fixes: 098c2cd281409 ("ASoC  ASoC: Intel: Atom: add 24-bit support for 
media playback and capture")
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>