[alsa-devel] [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;
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 = {
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
On 9/5/19 12:48 AM, Takashi Iwai wrote:
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.
It's not that crazy, this is supported in the HDaudio spec:
Sample Base Rate Divisor (DIV): 000 = Divide by 1 (48 kHz, 44.1 kHz) 001 = Divide by 2 (24 kHz, 22.05 kHz)
I am not sure why 22.05 made the cut and not 24 kHz, they are both derived from common clocks with the same divider... Same for 11.025 and 12...
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
On Thu, 05 Sep 2019 15:11:43 +0200, Pierre-Louis Bossart wrote:
On 9/5/19 12:48 AM, Takashi Iwai wrote:
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.
It's not that crazy, this is supported in the HDaudio spec:
Sample Base Rate Divisor (DIV): 000 = Divide by 1 (48 kHz, 44.1 kHz) 001 = Divide by 2 (24 kHz, 22.05 kHz)
I am not sure why 22.05 made the cut and not 24 kHz, they are both derived from common clocks with the same divider... Same for 11.025 and 12...
I'm not against supporting it. It'd be fine if it were the changes that are applied only to HD-audio driver side. What I'm against is to change the ALSA PCM core. It's not necessarily done there at all.
thanks,
Takashi
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.
It's not that crazy, this is supported in the HDaudio spec:
Sample Base Rate Divisor (DIV): 000 = Divide by 1 (48 kHz, 44.1 kHz) 001 = Divide by 2 (24 kHz, 22.05 kHz)
I am not sure why 22.05 made the cut and not 24 kHz, they are both derived from common clocks with the same divider... Same for 11.025 and 12...
I'm not against supporting it. It'd be fine if it were the changes that are applied only to HD-audio driver side. What I'm against is to change the ALSA PCM core. It's not necessarily done there at all.
Humm, out of curiosity what is the issue here? Would this addition break anything? I don't personally care too much but I've never quite understood why the ALSA core only defined a subset of 'common' rates.
On Thu, 05 Sep 2019 16:00:51 +0200, Pierre-Louis Bossart 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.
It's not that crazy, this is supported in the HDaudio spec:
Sample Base Rate Divisor (DIV): 000 = Divide by 1 (48 kHz, 44.1 kHz) 001 = Divide by 2 (24 kHz, 22.05 kHz)
I am not sure why 22.05 made the cut and not 24 kHz, they are both derived from common clocks with the same divider... Same for 11.025 and 12...
I'm not against supporting it. It'd be fine if it were the changes that are applied only to HD-audio driver side. What I'm against is to change the ALSA PCM core. It's not necessarily done there at all.
Humm, out of curiosity what is the issue here? Would this addition break anything? I don't personally care too much but I've never quite understood why the ALSA core only defined a subset of 'common' rates.
It's simply a policy that we don't add a thing just because one driver wants for some reason possibly no one would actually use.
In general such a core stuff is changed only when it has to be and inevitably necessary, and/or it'd be benefit for all the rest and majority users.
thanks,
Takashi
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 */
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 */
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;val |= AC_FMT_BITS_24;break;
if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)val |= AC_FMT_BITS_32;else if (maxbps >= 24)val |= AC_FMT_BITS_24;elseval |= AC_FMT_BITS_20;
val |= AC_FMT_BITS_32;-- 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;val |= AC_FMT_BITS_24;break;
if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE)val |= AC_FMT_BITS_32;else if (maxbps >= 24)val |= AC_FMT_BITS_24;elseval |= AC_FMT_BITS_20;
val |= AC_FMT_BITS_32;-- 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 (4)
-
Harlozinski, Pawel -
Pawel Harlozinski
-
Pierre-Louis Bossart -
Takashi Iwai