On 28/02/2019 14:15, Russell King - ARM Linux admin wrote:
> Hi all,
>
> While looking at hdmi-codec issues, I spotted this code:
>
> static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params,
> struct snd_soc_dai *dai)
> {
> ...
> if (params_width(params) > 24)
> params->msbits = 24;
>
> params->msbits is a parameter that is negotiated and refined by
> libasound, and is part of the ALSA constraint system. The "Writing an
> ALSA driver" documentation says about the hw_params callback:
>
> "This is called when the hardware parameter (``hw_params``) is set up
> by the application, that is, once when the buffer size, the period
> size, the format, etc. are defined for the pcm substream."
>
> which suggests we should only be reading the parameters, not writing
> to them.
>
> What's more is that the msbits is a parameter that is refined with
> userspace, so surely the above should be a declared constraint?
>
Certainly not a constraint. While HDMI can only pass up to 24-bit per
sample audio the most (or at least the two I have worked with) encoders
can take 32-bit wide (and probably wider) samples trough i2s. So the
system can play 32-bit samples, just the 4 LSB is ignored.
Actually there is very little difference between the case of playing
32-bit stereo audio with matching i2s bclk ratio, and playing 24-bit
stereo audio with 64-bit bclk ratio. Especially when the 4 LSB is anyway
ignored like in HDMI audio case.
> Digging a bit deeper, ASoC passes a private copy of the params to each
> codec - a copy is made, then fixups for TDM slots are applied, followed
> by any topology fixups by the DAI link (be_hw_params_fixup) before the
> codec driver's hw_params() callback is made. Afterwards, ASoC reads
> back the rate, channels and physical (memory) width and stores them
> in the codec's DAI structure. The msbits are not read.
>
> hdmi-codec also seems to do nothing with the msbits parameter other
> than the above code.
>
> So, all in all, it seems that the above code limiting msbits is
> redundant - nothing will read this modified value. Can we kill it?
>
It certainly looks that way, so yes. In any case struct
snd_soc_dai_driver .playback.sig_bits = 24 should be all that is needed.
Regards,
Jyri
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
