| Message ID | 20190905053302.9262-1-pawel.harlozinski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/3] ALSA:hda: Simplify and clear calculating SDxFMT | expand |
On Thu, 05 Sep 2019 07:32:59 +0200, Pawel Harlozinski wrote: > > Set SDxFMT based only on given format, due to maxbps not always being set. > Split cases for formats 20,24,32 bits. > For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so it will end up > with same mask. This function corresponds to snd_hdac_query_supported_pcm(), so this patch breaks. Basically the cases of 20 and 24 are superfluous and can be dropped. It's there just to be sure. Takashi > > 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 b26cc93e7e10..add758e11b85 100644 > --- a/sound/hda/hdac_device.c > +++ b/sound/hda/hdac_device.c > @@ -759,14 +759,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 >
Thanks for noticing ! Indeed snd_hdac_query_supported_pcm() also should be aligned. Could you help a bit there with explaining background of current implementation ? I'm wondering why SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE is set instead of SNDRV_PCM_FMTBIT_S32_LE in case of using 32 bits and AC_WCAP_DIGITAL. Whats more, bps looks like redundant information there, as pcm formats have width defined in sound/core/pcm-misc.c. I would leave 20 and 24 bits per sample there, as those are supportedformats for stream DMA. Regards, Paweł On 9/5/2019 7:47 AM, Takashi Iwai wrote: > On Thu, 05 Sep 2019 07:32:59 +0200, > Pawel Harlozinski wrote: >> Set SDxFMT based only on given format, due to maxbps not always being set. >> Split cases for formats 20,24,32 bits. >> For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so it will end up >> with same mask. > This function corresponds to snd_hdac_query_supported_pcm(), so this > patch breaks. > > Basically the cases of 20 and 24 are superfluous and can be dropped. > It's there just to be sure. > > > Takashi > >> 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 b26cc93e7e10..add758e11b85 100644 >> --- a/sound/hda/hdac_device.c >> +++ b/sound/hda/hdac_device.c >> @@ -759,14 +759,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 >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, 06 Sep 2019 15:41:15 +0200, Harlozinski, Pawel wrote: > > Thanks for noticing ! > > Indeed snd_hdac_query_supported_pcm() also should be aligned. > Could you help a bit there with explaining background of current > implementation ? > I'm wondering why SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE is set instead of > SNDRV_PCM_FMTBIT_S32_LE in case of using 32 bits and AC_WCAP_DIGITAL. That's some heuristic; if the device requires really 32bit, it's most likely not real 32bit but with the IEC status bits. This never has been a problem, so there is no reason to change it. Of course, if you have a real device that requires different format, then we can fix that. > Whats more, bps looks like redundant information there, as pcm formats have > width defined in sound/core/pcm-misc.c. > > I would leave 20 and 24 bits per sample there, as those are supported formats > for stream DMA. The HD-audio spec 20 and 24 bits are MSB-aligned, AFAIK, so they are not what you supposed with S20_LE or S24_LE format. Again, if you have really some device that behaves differently, it's a different story. In general, this kind of "cleanup" would easily lead to some breakage, and we have to be really careful. thanks, Takashi > > Regards, > Paweł > > On 9/5/2019 7:47 AM, Takashi Iwai wrote: > > On Thu, 05 Sep 2019 07:32:59 +0200, > Pawel Harlozinski wrote: > > Set SDxFMT based only on given format, due to maxbps not always being set. > Split cases for formats 20,24,32 bits. > For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so it will end up > with same mask. > > This function corresponds to snd_hdac_query_supported_pcm(), so this > patch breaks. > > Basically the cases of 20 and 24 are superfluous and can be dropped. > It's there just to be sure. > > Takashi > > 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 b26cc93e7e10..add758e11b85 100644 > --- a/sound/hda/hdac_device.c > +++ b/sound/hda/hdac_device.c > @@ -759,14 +759,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 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > >
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index b26cc93e7e10..add758e11b85 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -759,14 +759,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;
Set SDxFMT based only on given format, due to maxbps not always being set. Split cases for formats 20,24,32 bits. For format SNDRV_PCM_FORMAT_FLOAT_LE width is equal 32 so it will end up with same mask. Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com> --- sound/hda/hdac_device.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)