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

["Harlozinski, Pawel" <pawel.harlozinski@linux.intel.com>]

> 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