| Message ID | 20240628122429.2018059-2-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: update sample rate definition for eARC | expand |
Hi Jerome,
kernel test robot noticed the following build errors:
[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.10-rc5 next-20240628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/ALSA-pcm-add-support-for-128kHz-sample-rate/20240629-201915
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link: https://lore.kernel.org/r/20240628122429.2018059-2-jbrunet%40baylibre.com
patch subject: [PATCH 1/3] ALSA: pcm: add support for 128kHz sample rate
config: i386-buildonly-randconfig-004-20240630
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406300718.iL828kaG-lkp@intel.com/
All errors (new ones prefixed by >>):
>> sound/usb/caiaq/audio.c:179:2: error: #error "Change this table"
#error "Change this table"
^~~~~
vim +179 sound/usb/caiaq/audio.c
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 176
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 177 /* this should probably go upstream */
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 178 #if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 @179 #error "Change this table"
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 180 #endif
523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 181
Hi Jerome,
kernel test robot noticed the following build errors:
[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.10-rc5 next-20240628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/ALSA-pcm-add-support-for-128kHz-sample-rate/20240629-201915
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link: https://lore.kernel.org/r/20240628122429.2018059-2-jbrunet%40baylibre.com
patch subject: [PATCH 1/3] ALSA: pcm: add support for 128kHz sample rate
config: x86_64-buildonly-randconfig-003-20240630
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build):
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406300810.fyflTsWJ-lkp@intel.com/
All errors (new ones prefixed by >>):
>> sound/usb/caiaq/audio.c:179:2: error: "Change this table"
179 | #error "Change this table"
| ^
1 error generated.
vim +179 sound/usb/caiaq/audio.c
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 176
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 177 /* this should probably go upstream */
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 178 #if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 @179 #error "Change this table"
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 180 #endif
523f1dce37434a9 sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 181
On Sun 30 Jun 2024 at 07:29, kernel test robot <lkp@intel.com> wrote: > Hi Jerome, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on tiwai-sound/for-next] > [also build test ERROR on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.10-rc5 next-20240628] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/ALSA-pcm-add-support-for-128kHz-sample-rate/20240629-201915 > base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next > patch link: https://lore.kernel.org/r/20240628122429.2018059-2-jbrunet%40baylibre.com > patch subject: [PATCH 1/3] ALSA: pcm: add support for 128kHz sample rate > config: i386-buildonly-randconfig-004-20240630 > compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 > reproduce (this is a W=1 build): > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202406300718.iL828kaG-lkp@intel.com/ > > All errors (new ones prefixed by >>): > >>> sound/usb/caiaq/audio.c:179:2: error: #error "Change this table" > #error "Change this table" > ^~~~~ > > > vim +179 sound/usb/caiaq/audio.c > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 176 > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 177 /* this should probably go upstream */ > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 178 #if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 @179 #error "Change this table" > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 180 #endif > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 181 This file is not in mainline or https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next branch ... Don't know what to make of this error ?
On Sun, 30 Jun 2024 08:53:15 +0200, Jerome Brunet wrote: > > On Sun 30 Jun 2024 at 07:29, kernel test robot <lkp@intel.com> wrote: > > > Hi Jerome, > > > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on tiwai-sound/for-next] > > [also build test ERROR on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.10-rc5 next-20240628] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/ALSA-pcm-add-support-for-128kHz-sample-rate/20240629-201915 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next > > patch link: https://lore.kernel.org/r/20240628122429.2018059-2-jbrunet%40baylibre.com > > patch subject: [PATCH 1/3] ALSA: pcm: add support for 128kHz sample rate > > config: i386-buildonly-randconfig-004-20240630 > > compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 > > reproduce (this is a W=1 build): > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202406300718.iL828kaG-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > >>> sound/usb/caiaq/audio.c:179:2: error: #error "Change this table" > > #error "Change this table" > > ^~~~~ > > > > > > vim +179 sound/usb/caiaq/audio.c > > > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 176 > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 177 /* this should probably go upstream */ > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 178 #if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12 > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 @179 #error "Change this table" > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 180 #endif > > 523f1dce37434a sound/usb/caiaq/caiaq-audio.c Daniel Mack 2007-03-26 181 > > This file is not in mainline or > https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next > branch ... It's sound/usb/caiaq/audio.c. Takashi
On Mon 01 Jul 2024 at 16:07, Takashi Iwai <tiwai@suse.de> wrote: > On Mon, 01 Jul 2024 10:50:02 +0200, > Amadeusz Sławiński wrote: >> >> On 6/28/2024 2:23 PM, Jerome Brunet wrote: >> > The usual sample rate possible on an SPDIF link are >> > 32k, 44.1k, 48k, 88.2k, 96k, 172.4k and 192k. >> > >> > With higher bandwidth variant, such as eARC, and the introduction of 8 >> > channels mode, the spdif frame rate may be multiplied by 4. This happens >> > when the interface use an IEC958_SUBFRAME format. >> > >> > The spdif 8 channel mode rate list is: >> > 128k, 176.4k, 192k, 352.8k, 384k, 705.4k and 768k. >> > >> > All are already supported by ASLA expect for the 128kHz one. >> > Add support for it but do not insert it the SNDRV_PCM_RATE_8000_192000 >> > macro. Doing so would silently add 128k support to a lot of HW which >> > probably do not support it. >> > >> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> > --- >> >> From what I remember the recommendation is to not add new rates, but >> use SNDRV_PCM_RATE_KNOT for all rates not included already. > > In general yes -- unless the new rate is used for significant amount > of drivers. > > So this case is a sort of on a border line; if it's only for ASoC > SPDIF codec driver, I'd rather implement with an extra rate list > instead of extending the common bits (that has some potential risks by > breaking the existing numbers). At the moment it would be used by ASoC spdif codec yes (and with Amlogic eARC support reasonnably soon, hopefully) However it is likely to be a common rate of any derivative of an IEC958 interface, with a sufficiently high bandwidth. I suspect there might be more of those in the future. Also, it is not an exotic rate for some obscure codec no one really has. It is part of specified interface that every TV with HDMI 2 is likely to have nowadays. This is why I thought it made sense to add it to the usual list. It is the only rate missing, everything else is already there. Changing the spdif codecs with SNDRV_PCM_RATE_KNOT and a custom rate list is doable I suppose, if the new ID is not OK. BTW, I tried not changing the existing numbers and add 128k last but that broke. I guess something requires the IDs to be ordered. I did not check this further since updating the IDs worked fine (for me, at least :) ) > OTOH, if we can take this for further > cleanups of the existing requirement of 128khz rate, we can go with > it. > Apart from the problem reported in sound/usb/caiaq/audio.c, is there another clean up expected ? > > thanks, > > Takashi
>> >> Apart from the problem reported in sound/usb/caiaq/audio.c, is there >> another clean up expected ? > > The change for caiaq/audio.c is rather a "fix" :) > As a cleanup, I meant, whether this extension can be applied to the > other existing drivers that already use 128kHz with RATE_KNOT and an > extra list. Grepping in sound/ for 128000, I've found only 3 files which could benefit from solely adding 128kHz to the defined rates: * sound/pci/cmipci.c * sound/pci/rme9652/hdsp.c * sound/pci/rme9652/hdspm.c The rest are unsing other rates which require the use of RATE_KNOT. The most regular rates being 12kHz and 24kHz. Adding those as well could help in: * sound/soc/codecs/adau1977.c * sound/soc/fsl/fsl_asrc.c * sound/soc/fsl/fsl_easrc.c * sound/soc/intel/avs/pcm.c I admit that's a fairly low number of drivers, maybe it is not worth it at this stage. Takashi, Mark, what is your preference ? Should I: * tweak the spdif codec to use RATE_KNOT ? * add just 128kHz, fixing the 3 file above ? * add 12 and 24kHz as well ? I don't really mind one way or the other. > > > thanks, > > Takashi
On Fri, 09 Aug 2024 10:29:05 +0200, Jerome Brunet wrote: > > > >> > >> Apart from the problem reported in sound/usb/caiaq/audio.c, is there > >> another clean up expected ? > > > > The change for caiaq/audio.c is rather a "fix" :) > > As a cleanup, I meant, whether this extension can be applied to the > > other existing drivers that already use 128kHz with RATE_KNOT and an > > extra list. > > Grepping in sound/ for 128000, I've found only 3 files which could > benefit from solely adding 128kHz to the defined rates: > > * sound/pci/cmipci.c > * sound/pci/rme9652/hdsp.c > * sound/pci/rme9652/hdspm.c > > The rest are unsing other rates which require the use of RATE_KNOT. > The most regular rates being 12kHz and 24kHz. Adding those as well could > help in: > > * sound/soc/codecs/adau1977.c > * sound/soc/fsl/fsl_asrc.c > * sound/soc/fsl/fsl_easrc.c > * sound/soc/intel/avs/pcm.c > > I admit that's a fairly low number of drivers, maybe it is not worth it > at this stage. > > Takashi, Mark, what is your preference ? Should I: > * tweak the spdif codec to use RATE_KNOT ? > * add just 128kHz, fixing the 3 file above ? > * add 12 and 24kHz as well ? > > I don't really mind one way or the other. If there are multiple instances, it's fine to extend the standards. Then we can clean up them as well. thanks, Takashi
On Fri, Aug 09, 2024 at 10:42:37AM +0200, Takashi Iwai wrote: > Jerome Brunet wrote: > > Takashi, Mark, what is your preference ? Should I: > > * tweak the spdif codec to use RATE_KNOT ? > > * add just 128kHz, fixing the 3 file above ? > > * add 12 and 24kHz as well ? > > I don't really mind one way or the other. > If there are multiple instances, it's fine to extend the standards. > Then we can clean up them as well. I tend to agree, it seems like an obvious thing that people will be wanting to use and there's already multiple users appearing.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 3edd7a7346da..9cda92b34eda 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -116,12 +116,13 @@ struct snd_pcm_ops { #define SNDRV_PCM_RATE_64000 (1U<<8) /* 64000Hz */ #define SNDRV_PCM_RATE_88200 (1U<<9) /* 88200Hz */ #define SNDRV_PCM_RATE_96000 (1U<<10) /* 96000Hz */ -#define SNDRV_PCM_RATE_176400 (1U<<11) /* 176400Hz */ -#define SNDRV_PCM_RATE_192000 (1U<<12) /* 192000Hz */ -#define SNDRV_PCM_RATE_352800 (1U<<13) /* 352800Hz */ -#define SNDRV_PCM_RATE_384000 (1U<<14) /* 384000Hz */ -#define SNDRV_PCM_RATE_705600 (1U<<15) /* 705600Hz */ -#define SNDRV_PCM_RATE_768000 (1U<<16) /* 768000Hz */ +#define SNDRV_PCM_RATE_128000 (1U<<11) /* 128000Hz */ +#define SNDRV_PCM_RATE_176400 (1U<<12) /* 176400Hz */ +#define SNDRV_PCM_RATE_192000 (1U<<13) /* 192000Hz */ +#define SNDRV_PCM_RATE_352800 (1U<<14) /* 352800Hz */ +#define SNDRV_PCM_RATE_384000 (1U<<15) /* 384000Hz */ +#define SNDRV_PCM_RATE_705600 (1U<<16) /* 705600Hz */ +#define SNDRV_PCM_RATE_768000 (1U<<17) /* 768000Hz */ #define SNDRV_PCM_RATE_CONTINUOUS (1U<<30) /* continuous range */ #define SNDRV_PCM_RATE_KNOT (1U<<31) /* supports more non-continuous rates */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 521ba56392a0..87eeb9b7f54a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2410,13 +2410,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_768000 != 1 << 17 #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, 705600, 768000 + 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, + 96000, 128000, 176400, 192000, 352800, 384000, 705600, 768000, }; const struct snd_pcm_hw_constraint_list snd_pcm_known_rates = {
The usual sample rate possible on an SPDIF link are 32k, 44.1k, 48k, 88.2k, 96k, 172.4k and 192k. With higher bandwidth variant, such as eARC, and the introduction of 8 channels mode, the spdif frame rate may be multiplied by 4. This happens when the interface use an IEC958_SUBFRAME format. The spdif 8 channel mode rate list is: 128k, 176.4k, 192k, 352.8k, 384k, 705.4k and 768k. All are already supported by ASLA expect for the 128kHz one. Add support for it but do not insert it the SNDRV_PCM_RATE_8000_192000 macro. Doing so would silently add 128k support to a lot of HW which probably do not support it. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- include/sound/pcm.h | 13 +++++++------ sound/core/pcm_native.c | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-)