mbox series

[RFC,00/17] ALSA/ASoC: hda: Address format selection limitations and ambiguity

Message ID 20230811164853.1797547-1-cezary.rojewski@intel.com
Headers show
Series ALSA/ASoC: hda: Address format selection limitations and ambiguity | expand

Message

Cezary Rojewski Aug. 11, 2023, 4:48 p.m. UTC 
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 three 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.

Note: No testing done on other drivers yet. First results from
snd_hda_intel and snd_soc_skl will be here on Monday. Will trigger SOF
cycle through github PR.


[1]: https://lore.kernel.org/alsa-devel/20190905053302.9262-1-pawel.harlozinski@linux.intel.com/
[2]: https://github.com/alsa-project/alsa-utils/pull/228
[3]: https://github.com/alsa-project/alsa-lib/pull/342

Cezary Rojewski (17):
  ALSA: pcm: Introduce MSBITS subformat interface
  ALSA: pcm: Honor subformat when configuring substream
  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: Unhardcode HDAudio BE DAI drivers description
  ASoC: Intel: avs: Kill S24_LE in HDAudio streaming

 include/sound/hda_codec.h         |   5 +-
 include/sound/hdaudio.h           |  12 +--
 include/sound/pcm.h               |   8 ++
 include/sound/pcm_params.h        |   2 +
 include/sound/soc.h               |   1 +
 include/uapi/sound/asound.h       |   5 +-
 sound/core/pcm_lib.c              |  30 ++++++
 sound/core/pcm_misc.c             |  23 +++++
 sound/core/pcm_native.c           |   5 +-
 sound/hda/hdac_device.c           | 153 +++++++++++++++++++++---------
 sound/pci/hda/hda_codec.c         |   2 +
 sound/pci/hda/hda_controller.c    |  10 +-
 sound/pci/hda/patch_ca0132.c      |   3 +-
 sound/pci/hda/patch_hdmi.c        |   6 +-
 sound/soc/codecs/hda-dai.c        |   6 +-
 sound/soc/codecs/hda.c            |   2 +
 sound/soc/codecs/hdac_hda.c       |   8 +-
 sound/soc/codecs/hdac_hdmi.c      |  10 +-
 sound/soc/intel/avs/loader.c      |   4 +-
 sound/soc/intel/avs/path.c        |   2 +-
 sound/soc/intel/avs/pcm.c         |  60 ++++++------
 sound/soc/intel/avs/probes.c      |   3 +-
 sound/soc/intel/avs/topology.c    |   9 +-
 sound/soc/intel/skylake/skl-pcm.c |  11 ++-
 sound/soc/soc-pcm.c               |  17 ++++
 sound/soc/sof/intel/hda-dai-ops.c |   5 +-
 tools/include/uapi/sound/asound.h |   5 +-
 27 files changed, 290 insertions(+), 117 deletions(-)

Comments

Takashi Iwai Aug. 14, 2023, 2:41 p.m. UTC | #1 
On Fri, 11 Aug 2023 18:48:41 +0200,
Cezary Rojewski wrote:
> 
> Introduce a set of functions that facilite SDxFMT-related calculations
> in atomic manner:
> 
> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
> unsigned and BE friends are invalid from HDAudio perspective but still
> can be specified as function argument due to compatibility reasons.

Which compatibility reason?  Those formats should have been never used
for HD-audio codecs at the first place, so shouldn't it rather return
an error?


thanks,

Takashi
Cezary Rojewski Aug. 14, 2023, 2:47 p.m. UTC | #2 
On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> On Fri, 11 Aug 2023 18:48:41 +0200,
> Cezary Rojewski wrote:
>>
>> Introduce a set of functions that facilite SDxFMT-related calculations
>> in atomic manner:
>>
>> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
>> unsigned and BE friends are invalid from HDAudio perspective but still
>> can be specified as function argument due to compatibility reasons.
> 
> Which compatibility reason?  Those formats should have been never used
> for HD-audio codecs at the first place, so shouldn't it rather return
> an error?

In regard to avs-driver we can "force" the S24_LE out, no problem. 
However, I do not believe the same is true for the skylake-driver. I 
agree, S24_LE and its friends should not have been used, but they were.


Czarek
Takashi Iwai Aug. 14, 2023, 2:59 p.m. UTC | #3 
On Mon, 14 Aug 2023 16:47:05 +0200,
Cezary Rojewski wrote:
> 
> On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> > On Fri, 11 Aug 2023 18:48:41 +0200,
> > Cezary Rojewski wrote:
> >> 
> >> Introduce a set of functions that facilite SDxFMT-related calculations
> >> in atomic manner:
> >> 
> >> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
> >> unsigned and BE friends are invalid from HDAudio perspective but still
> >> can be specified as function argument due to compatibility reasons.
> > 
> > Which compatibility reason?  Those formats should have been never used
> > for HD-audio codecs at the first place, so shouldn't it rather return
> > an error?
> 
> In regard to avs-driver we can "force" the S24_LE out, no
> problem. However, I do not believe the same is true for the
> skylake-driver. I agree, S24_LE and its friends should not have been
> used, but they were.

Hm, if those formats are actually passed to the codec side, then it's
rather a bug, I suppose.  It can be used for the controller where DSP
converts the format, though.


Takashi