[PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[Pawel Harlozinski]

Fix setting SDnFMT based on High Definition Audio Specification Rev. 1.0a page 48.

Bits per Sample (BITS):
000 = 8 bits. The data will be packed in memory in 8-bit containers on 16-bit boundaries.
001 = 16 bits. The data will be packed in memory in 16-bit containers on 16-bit boundaries.
010 = 20 bits. The data will be packed in memory in 32-bit containers on 32-bit boundaries.
011 = 24 bits. The data will be packed in memory in 32-bit containers on 32-bit boundaries.
100 = 32 bits. The data will be packed in memory in 32-bit containers on 32-bit boundaries.
101-111 = Reserved

Set SDnFMT depending on which format was given, as maxbps only describes container size.
Henceforth split cases for formats 20, 24, 32 bits.
For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so as previously it will end up with mask for 32 bits.

Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
---
 sound/hda/hdac_device.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 333220f0f8af..2ccf79866f99 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -762,14 +762,13 @@ unsigned int snd_hdac_calc_stream_format(unsigned int rate,
 		val |= AC_FMT_BITS_16;
 		break;
 	case 20:
+		val |= AC_FMT_BITS_20;
+		break;
 	case 24:
+		val |= AC_FMT_BITS_24;
+		break;
 	case 32:
-		if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)
-			val |= AC_FMT_BITS_32;
-		else if (maxbps >= 24)
-			val |= AC_FMT_BITS_24;
-		else
-			val |= AC_FMT_BITS_20;
+		val |= AC_FMT_BITS_32;
 		break;
 	default:
 		return 0;
-- 
2.17.1

Re: [PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[Kai Vehmanen]

Hey,

On Mon, 24 Aug 2020, Pawel Harlozinski wrote:

> Set SDnFMT depending on which format was given, as maxbps only describes container size.

hmm, I'm not entirely sure that is correct. Usage may be a bit varied, but 
most places in existing code, "maxbps" is treated as number of significant 
bits, not the container size. E.g. in hdac_hda.c:

»       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
»       »       maxbps = dai->driver->playback.sig_bits;
»       else
»       »       maxbps = dai->driver->capture.sig_bits;

It would seem "maxbps" is a bit superfluous given the same information can 
be relayed in "format" as well. But currently it's still used. E.g. if you 
look at snd_hdac_query_supported_pcm(), if codec reports 24bit support, 
format is always set to SNDRV_PCM_FMTBIT_S32_LE, even if only 24bit are 
valid. So snd_pcm_format_width() will not return the expected significant 
bits info, but you have to use "maxbps". So original code seems correct 
(or at least you'd need to update both places).

Br, Kai

Re: [PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[Takashi Iwai]

On Mon, 24 Aug 2020 14:16:26 +0200,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Mon, 24 Aug 2020, Pawel Harlozinski wrote:
> 
> > Set SDnFMT depending on which format was given, as maxbps only describes container size.
> 
> hmm, I'm not entirely sure that is correct. Usage may be a bit varied, but 
> most places in existing code, "maxbps" is treated as number of significant 
> bits, not the container size. E.g. in hdac_hda.c:
> 
> »       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> »       »       maxbps = dai->driver->playback.sig_bits;
> »       else
> »       »       maxbps = dai->driver->capture.sig_bits;
> 
> It would seem "maxbps" is a bit superfluous given the same information can 
> be relayed in "format" as well. But currently it's still used. E.g. if you 
> look at snd_hdac_query_supported_pcm(), if codec reports 24bit support, 
> format is always set to SNDRV_PCM_FMTBIT_S32_LE, even if only 24bit are 
> valid. So snd_pcm_format_width() will not return the expected significant 
> bits info, but you have to use "maxbps". So original code seems correct 
> (or at least you'd need to update both places).

Hm, we need to check the call pattern, then.  The maxbps passed to
this function was supposed to be the value obtained from
snd_hdac_query_supported_pcm(), i.e. the codec capability.

But, basically this patch wouldn't change any practical behavior.  In
the current code, snd_pcm_format_width() can be never 20 or 24,
because the 24 and 24bit supports are also with SNDRV_PCM_FMT_S32_LE.
That is, the cases 20 and 24 there are superfluous from the
beginning (although the checks of maxbps are still needed).

Instead, what we could improve is:
- Set up the proper msbits hw_constraint to reflect the maxbps value
- Choose the right AC_FMT_BITS_* depending on the hw_params msbits

We may change the query function not to return a single maxbps value
but rather storing the raw PCM parameter value (AC_SUPPCM_*), and pass
it at re-encoding the format value, too, if we want to make things
perfect.


thanks,

Takashi

Re: [PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[Harlozinski, Pawel]

Hey!

Thanks for Your input!

I've created that patch because our validation is actually checking if 
values
in SDnFMT are matching their expectations, and they've found  it 
indicates 32 bits in 32 container while playing 24 bits in 32 container.
This could be fixed without touching checks of maxbps:

	switch (snd_pcm_format_width(format)) {
	case 8:
		val |= AC_FMT_BITS_8;
		break;
	case 16:
		val |= AC_FMT_BITS_16;
		break;
	case 20:
		val |= AC_FMT_BITS_20;
		break;
	case 24:
		if (maxbps >= 24)
			val |= AC_FMT_BITS_24;
		else
			val |= AC_FMT_BITS_20;
		break;
	case 32:
		if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)
			val |= AC_FMT_BITS_32;
		else if (maxbps >= 24)
			val |= AC_FMT_BITS_24;
		else
			val |= AC_FMT_BITS_20;
		break;
	default:
		return 0;
	}


I've simplified that because maxbps seems redundant here - thansk for 
catching Kai!
Although reason of  usage maxbps is still not clear (at least for me).

On 8/25/2020 10:25 AM, Takashi Iwai wrote:

> On Mon, 24 Aug 2020 14:16:26 +0200,
> Kai Vehmanen wrote:
>> Hey,
>>
>> On Mon, 24 Aug 2020, Pawel Harlozinski wrote:
>>
>>> Set SDnFMT depending on which format was given, as maxbps only describes container size.
>> hmm, I'm not entirely sure that is correct. Usage may be a bit varied, but
>> most places in existing code, "maxbps" is treated as number of significant
>> bits, not the container size. E.g. in hdac_hda.c:
>>
>> »       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> »       »       maxbps = dai->driver->playback.sig_bits;
>> »       else
>> »       »       maxbps = dai->driver->capture.sig_bits;
>>
>> It would seem "maxbps" is a bit superfluous given the same information can
>> be relayed in "format" as well. But currently it's still used. E.g. if you
>> look at snd_hdac_query_supported_pcm(), if codec reports 24bit support,
>> format is always set to SNDRV_PCM_FMTBIT_S32_LE even if only 24bit are valid.
So, for me looks like place where we can align with actual format, right ?
>>   So snd_pcm_format_width() will not return the expected significant
>> bits info, but you have to use "maxbps". So original code seems correct
>> (or at least you'd need to update both places).

> Hm, we need to check the call pattern, then.  The maxbps passed to
> this function was supposed to be the value obtained from
> snd_hdac_query_supported_pcm(), i.e. the codec capability.
Here I'm also not sure if we should just "cut" format  in 
snd_hdac_calc_stream_format (eg. 32 to 24) if codec does not support 32?

> But, basically this patch wouldn't change any practical behavior.  In
> the current code, snd_pcm_format_width() can be never 20 or 24,
> because the 24 and 24bit supports are also with SNDRV_PCM_FMT_S32_LE.
> That is, the cases 20 and 24 there are superfluous from the
> beginning (although the checks of maxbps are still needed
>
> Instead, what we could improve is:
> - Set up the proper msbits hw_constraint to reflect the maxbps value
> - Choose the right AC_FMT_BITS_* depending on the hw_params msbitsWe may change the query function not to return a single maxbps value
> but rather storing the raw PCM parameter value (AC_SUPPCM_*), and pass
> it at re-encoding the format value, too, if we want to make
>
> thanks,
>
> Takashi

Re: [PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification

[Harlozinski, Pawel]

> Hey!
>
> Thanks for Your input!
>
> I've created that patch because our validation is actually checking if 
> values
> in SDnFMT are matching their expectations, and they've found  it 
> indicates 32 bits in 32 container while playing 24 bits in 32 container.
> This could be fixed without touching checks of maxbps:
>
> 	switch (snd_pcm_format_width(format)) {
> 	case 8:
> 		val |= AC_FMT_BITS_8;
> 		break;
> 	case 16:
> 		val |= AC_FMT_BITS_16;
> 		break;
> 	case 20:
> 		val |= AC_FMT_BITS_20;
> 		break;
> 	case 24:
> 		if (maxbps >= 24)
> 			val |= AC_FMT_BITS_24;
> 		else
> 			val |= AC_FMT_BITS_20;
> 		break;
> 	case 32:
> 		if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)
> 			val |= AC_FMT_BITS_32;
> 		else if (maxbps >= 24)
> 			val |= AC_FMT_BITS_24;
> 		else
> 			val |= AC_FMT_BITS_20;
> 		break;
> 	default:
> 		return 0;
> 	}
>
>
> I've simplified that because maxbps seems redundant here - thansk for 
> catching Kai!
> Although reason of  usage maxbps is still not clear (at least for me).
>
> On 8/25/2020 10:25 AM, Takashi Iwai wrote:
>
>> On Mon, 24 Aug 2020 14:16:26 +0200,
>> Kai Vehmanen wrote:
>>> Hey,
>>>
>>> On Mon, 24 Aug 2020, Pawel Harlozinski wrote:
>>>
>>>> Set SDnFMT depending on which format was given, as maxbps only describes container size.
>>> hmm, I'm not entirely sure that is correct. Usage may be a bit varied, but
>>> most places in existing code, "maxbps" is treated as number of significant
>>> bits, not the container size. E.g. in hdac_hda.c:
>>>
>>> »       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>> »       »       maxbps = dai->driver->playback.sig_bits;
>>> »       else
>>> »       »       maxbps = dai->driver->capture.sig_bits;
>>>
>>> It would seem "maxbps" is a bit superfluous given the same information can
>>> be relayed in "format" as well. But currently it's still used. E.g. if you
>>> look at snd_hdac_query_supported_pcm(), if codec reports 24bit support,
>>> format is always set to SNDRV_PCM_FMTBIT_S32_LE even if only 24bit are valid.
> So, for me looks like place where we can align with actual format, 
> right ?
>>>   So snd_pcm_format_width() will not return the expected significant
>>> bits info, but you have to use "maxbps". So original code seems correct
>>> (or at least you'd need to update both places).
>
>> Hm, we need to check the call pattern, then.  The maxbps passed to
>> this function was supposed to be the value obtained from
>> snd_hdac_query_supported_pcm(), i.e. the codec capability.
> Here I'm also not sure if we should just "cut" format  in 
> snd_hdac_calc_stream_format (eg. 32 to 24) if codec does not support 32?
>
>> But, basically this patch wouldn't change any practical behavior.  In
>> the current code, snd_pcm_format_width() can be never 20 or 24,
>> because the 24 and 24bit supports are also with SNDRV_PCM_FMT_S32_LE.
>> That is, the cases 20 and 24 there are superfluous from the
>> beginning (although the checks of maxbps are still needed
>>
>> Instead, what we could improve is:
>> - Set up the proper msbits hw_constraint to reflect the maxbps value
>> - Choose the right AC_FMT_BITS_* depending on the hw_params msbitsWe may change the query function not to return a single maxbps value
>> but rather storing the raw PCM parameter value (AC_SUPPCM_*), and pass
>> it at re-encoding the format value, too, if we want to make
>>
>> thanks,
>>
>> Takashi