[RFC PATCH 1/3] ALSA:hda: Simplify and clear calculating SDxFMT
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(-) 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
Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability with alsa library. Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com> --- include/sound/pcm.h | 1 + sound/core/pcm_native.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index bbe6eb1ff5d2..09d0a2a2dce8 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -119,6 +119,7 @@ struct snd_pcm_ops { #define SNDRV_PCM_RATE_192000 (1<<12) /* 192000Hz */ #define SNDRV_PCM_RATE_352800 (1<<13) /* 352800Hz */ #define SNDRV_PCM_RATE_384000 (1<<14) /* 384000Hz */ +#define SNDRV_PCM_RATE_24000 (1<<15) /* 24000Hz */ #define SNDRV_PCM_RATE_CONTINUOUS (1<<30) /* continuous range */ #define SNDRV_PCM_RATE_KNOT (1<<31) /* supports more non-continuos rates */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 11e653c8aa0e..f52f28e3edb1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2164,13 +2164,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params, return snd_interval_refine(hw_param_interval(params, rule->var), &t); } -#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 || SNDRV_PCM_RATE_24000 != 1 << 15 #error "Change this table" #endif static const unsigned int rates[] = { 5512, 8000, 11025, 16000, 22050, 32000, 44100, - 48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000 + 48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 24000 }; const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = { -- 2.17.1
On Thu, 05 Sep 2019 07:33:00 +0200, Pawel Harlozinski wrote:
Adds SNDRV_PCM_RATE_24000 at the bottom to keep backward compability with alsa library.
Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
No. Such a fancy rate has to be handled inside the driver locally instead of adding to the common rate. Takashi
--- include/sound/pcm.h | 1 + sound/core/pcm_native.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index bbe6eb1ff5d2..09d0a2a2dce8 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -119,6 +119,7 @@ struct snd_pcm_ops { #define SNDRV_PCM_RATE_192000 (1<<12) /* 192000Hz */ #define SNDRV_PCM_RATE_352800 (1<<13) /* 352800Hz */ #define SNDRV_PCM_RATE_384000 (1<<14) /* 384000Hz */ +#define SNDRV_PCM_RATE_24000 (1<<15) /* 24000Hz */
#define SNDRV_PCM_RATE_CONTINUOUS (1<<30) /* continuous range */ #define SNDRV_PCM_RATE_KNOT (1<<31) /* supports more non-continuos rates */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 11e653c8aa0e..f52f28e3edb1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2164,13 +2164,13 @@ static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params, return snd_interval_refine(hw_param_interval(params, rule->var), &t); }
-#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 +#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 || SNDRV_PCM_RATE_24000 != 1 << 15 #error "Change this table" #endif
static const unsigned int rates[] = { 5512, 8000, 11025, 16000, 22050, 32000, 44100, - 48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000 + 48000, 64000, 88200, 96000, 176400, 192000, 352800, 384000, 24000 };
const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = { -- 2.17.1
Adds rates 24kHz & 64kHz to allow proper calculation SDxFMT value. Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com> --- sound/hda/hdac_device.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index add758e11b85..72e00d93adb6 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -702,14 +702,16 @@ static struct hda_rate_tbl rate_bits[] = { { 11025, SNDRV_PCM_RATE_11025, HDA_RATE(44, 1, 4) }, { 16000, SNDRV_PCM_RATE_16000, HDA_RATE(48, 1, 3) }, { 22050, SNDRV_PCM_RATE_22050, HDA_RATE(44, 1, 2) }, + { 24000, SNDRV_PCM_RATE_24000, HDA_RATE(48, 1, 2) }, { 32000, SNDRV_PCM_RATE_32000, HDA_RATE(48, 2, 3) }, { 44100, SNDRV_PCM_RATE_44100, HDA_RATE(44, 1, 1) }, { 48000, SNDRV_PCM_RATE_48000, HDA_RATE(48, 1, 1) }, + { 64000, SNDRV_PCM_RATE_64000, HDA_RATE(48, 4, 3) }, { 88200, SNDRV_PCM_RATE_88200, HDA_RATE(44, 2, 1) }, { 96000, SNDRV_PCM_RATE_96000, HDA_RATE(48, 2, 1) }, { 176400, SNDRV_PCM_RATE_176400, HDA_RATE(44, 4, 1) }, { 192000, SNDRV_PCM_RATE_192000, HDA_RATE(48, 4, 1) }, -#define AC_PAR_PCM_RATE_BITS 11 +#define AC_PAR_PCM_RATE_BITS 13 /* up to bits 10, 384kHZ isn't supported properly */ /* not autodetected value */ -- 2.17.1
Adds rates 24kHz & 64kHz to allow proper calculation SDxFMT value. Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com> --- sound/hda/hdac_device.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index add758e11b85..72e00d93adb6 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -702,14 +702,16 @@ static struct hda_rate_tbl rate_bits[] = { { 11025, SNDRV_PCM_RATE_11025, HDA_RATE(44, 1, 4) }, { 16000, SNDRV_PCM_RATE_16000, HDA_RATE(48, 1, 3) }, { 22050, SNDRV_PCM_RATE_22050, HDA_RATE(44, 1, 2) }, + { 24000, SNDRV_PCM_RATE_24000, HDA_RATE(48, 1, 2) }, { 32000, SNDRV_PCM_RATE_32000, HDA_RATE(48, 2, 3) }, { 44100, SNDRV_PCM_RATE_44100, HDA_RATE(44, 1, 1) }, { 48000, SNDRV_PCM_RATE_48000, HDA_RATE(48, 1, 1) }, + { 64000, SNDRV_PCM_RATE_64000, HDA_RATE(48, 4, 3) }, { 88200, SNDRV_PCM_RATE_88200, HDA_RATE(44, 2, 1) }, { 96000, SNDRV_PCM_RATE_96000, HDA_RATE(48, 2, 1) }, { 176400, SNDRV_PCM_RATE_176400, HDA_RATE(44, 4, 1) }, { 192000, SNDRV_PCM_RATE_192000, HDA_RATE(48, 4, 1) }, -#define AC_PAR_PCM_RATE_BITS 11 +#define AC_PAR_PCM_RATE_BITS 13 /* up to bits 10, 384kHZ isn't supported properly */ /* not autodetected value */ -- 2.17.1
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:
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
On Thu, 05 Sep 2019 07:32:59 +0200, Pawel Harlozinski wrote: 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
participants (3)
-
Harlozinski, Pawel -
Pawel Harlozinski
-
Takashi Iwai