Re: [PATCH v5 00/16] ALSA/ASoC: hda: Address format selection limitations and ambiguity

[Takashi Iwai <tiwai@xxxxxxx>]

On Fri, 17 Nov 2023 13:05:54 +0100,
Cezary Rojewski wrote:
> 
> Patchset aims to address format selection restrictions present currently
> in the HDAudio library. Formats which we are concerned about are 20 and
> 24 valid bits per sample within 32 bit depth container. One may identify
> them as S20_LE and S24_LE except that those, according to comments found
> in include/uapi/sound/asound.h, are for LSB-aligned scenarios. HDAudio
> streams expect MSB-aligned data, no matter if we are speaking of HOST
> (SDxFMT) or LINK (PPLCxFMT) side - chapter 4.5.1 of the public HDAudio
> specification. In short, S20_LE and S24_LE are invalid options.
> 
> Right now, given the implementation of snd_hdac_query_supported_pcm() 
> within sound/hda/hdac_device.c, even if a codec responds with: "I
> support all possible formats specified within HDAudio specification",
> there will be no option to open a 20/32 or 24/32 stream. The kernel will
> force the stream to be opened using the highest available bit depth.
> 
> After discussing subject initially with Jaroslav and Takashi, suggestion
> was made to utilize 'subformat' option to address the problem. The
> eye-opening discussion begun much earlier though, in 2019 [1].
> 
> Paired with PRs for alsa-utils [2] and alsa-lib [3].
> 
> 
> Flow of changes:
> 
> The very first patch adds MSBITS subformat options to allow for granular
> 20/32, 24/32 and 32/32 format selection. The next two make sure
> subformat is actually honored during runtime. Most of that code is based
> on format-related API.
> 
> Follow up is upgrade to the hda stream-format interface - several
> functions are added to make the granular format selection simple in the
> HDAudio world. Core of the implementation is based on the existing
> snd_hdac_calc_stream_format(). The next ten patches are straightforward
> switch from one interface to another with cleanup of now-unsed function
> as a finishing touch.
> 
> Last but not least - the avs-driver, on which the problem analyzed and
> debugged, is updated to no longer acknowledge S24_LE as a valid format
> option.
> 
> Results with skylake-driver and snd_hda_intel show status quo on our
> RVPs. PR filed on SOF github shows promising results too [4].
> 
> 
> Changes in v5:
> - reworded 'bps' to 'bits' in all occurrences.
> - fixed an issue of MSBITS_MAX not being reported for S32_LE as reported
>   by Jaroslav
> - snd_pcm_subformat_width() has been inlined into its only user:
>   snd_pcm_hw_params_bits() as suggested by Jaroslav.
> - updated commit messege for patch 01 as it was out of date given the
>   recent updates.
> 
> Changes in v4:
> - fixed compilation issues in sof-driver, patch 12/16, reported by ikp
> - fixed sparse warnings in patch 01/16, reported by ikp
> - updated commit message for patch 03: "ASoC pcm: Honor subformat when
>   configuring runtime", as snd_pcm_hw_copy() is gone since revision v3.
> 
> Changes in v3:
> - merged the first two patches as suggested by Jaroslav
> - re-authored patch 01 to Jaroslav, added my Co-developed-by.
> - added Jaroslav' Co-developed-by to patch 02.
> - 'subformats' field now S32_LE-specific. Given the fact that it is the
>   only format currently requiring subformat-intervention, functionality
>   is narrowed to reduce amount of memory allocations and cleanup.
>   Suggested by Jaroslav.
> - note to the above: the hdaudio part converted 1:1 as requested, patch
>   02/16
> - note #2: alsa part converted to S32_LE-specific yet without addition
>   of the chicken bit. Instead, struct snd_pcm_hardware is updated with
>   u32 subformat mask to do the job.
> - ALSA-core additions in form of snd_pcm_subformat_width() and
>   snd_pcm_hw_params_bps() relocated from 01/16 to the user, patch 05.
> 
> Changes in v2:
> - patch 01/17, introduced struct snd_pcm_subformat which task is to
>   represent subformat-mask on per format basis. Expectation is that
>   manipulated arrays of subformats always end with a sentinel entry
> - patch 01/17, added snd_pcm_hw_copy() as the copying snd_pcm_hardware
>   becomes non-trivial
> - patch 02/17, added hw_rule that produces final subformat mask
>   based on provided formats as suggested by Jaroslav
> - patch 04/17, soc_pcm_hw_update_subformat() refactored as the subformat
>   intersection becomes non-trivial
> - relevant functions releasing resources occupied by hda_pcm and
>   snd_pcm_runtime updated to also kfree() subformats
> - except for 16/17, no changes to patches past 04/17, retaining acks for
>   these
> 
> Changes in v1:
> - fixed UBSAN due to missing snd_pcm_subformat_names[] entries for new
>   subformats
> - as HDMI stream capabilities are assigned on PCM open, patch 16/17 has
>   been updated to ignore such codecs for now. A separate patchset will
>   take care of this case
> - params_bps() reworded to snd_pcm_hw_params_bps()
> - fixed compilation issues in sof-driver, patch 13/17
> 
> 
> [1]: https://lore.kernel.org/alsa-devel/20190905053302.9262-1-pawel.harlozinski@xxxxxxxxxxxxxxx/
> [2]: https://github.com/alsa-project/alsa-utils/pull/228
> [3]: https://github.com/alsa-project/alsa-lib/pull/342
> [4]: https://github.com/thesofproject/linux/pull/4539
> 
> Cezary Rojewski (15):
>   ALSA: hda: Honor subformat when querying PCMs
>   ASoC: pcm: Honor subformat when configuring runtime
>   ALSA: hda: Upgrade stream-format infrastructure
>   ALSA: hda: Switch to new stream-format interface
>   ALSA: hda/hdmi: Switch to new stream-format interface
>   ALSA: hda/ca0132: Switch to new stream-format interface
>   ASoC: codecs: hda: Switch to new stream-format interface
>   ASoC: codecs: hdac_hda: Switch to new stream-format interface
>   ASoC: codecs: hdac_hdmi: Switch to new stream-format interface
>   ASoC: Intel Skylake: Switch to new stream-format interface
>   ASoC: SOF: Intel: Switch to new stream-format interface
>   ASoC: Intel: avs: Switch to new stream-format interface
>   ALSA: hda: Drop snd_hdac_calc_stream_format()
>   ASoC: Intel: avs: Kill S24_LE format
>   ASoC: Intel: avs: Unhardcode HDAudio BE DAI drivers description
> 
> Jaroslav Kysela (1):
>   ALSA: pcm: Introduce MSBITS subformat interface

Now the patch 1 became the one from Jaroslav, it's a good move.
Jaroslav, could you take a look at the rest patches?
I'm going to merge this in this week unless any objection comes up.


thanks,

Takashi