| Message ID | E1YdbKA-00085Q-LL@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Thu, 02 Apr 2015 10:22:06 +0100, Russell King wrote: > > Add a helper for the EDID like data structure, which is typically passed > from a HDMI adapter to its associated audio driver. This informs the > audio driver of the capabilities of the attached HDMI sink. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > include/sound/pcm_drm_eld.h | 6 +++ > sound/core/Kconfig | 3 ++ > sound/core/Makefile | 1 + > sound/core/pcm_drm_eld.c | 92 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 102 insertions(+) > create mode 100644 include/sound/pcm_drm_eld.h > create mode 100644 sound/core/pcm_drm_eld.c > > diff --git a/include/sound/pcm_drm_eld.h b/include/sound/pcm_drm_eld.h > new file mode 100644 > index 000000000000..93357b25d2e2 > --- /dev/null > +++ b/include/sound/pcm_drm_eld.h > @@ -0,0 +1,6 @@ > +#ifndef __SOUND_PCM_DRM_ELD_H > +#define __SOUND_PCM_DRM_ELD_H > + > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld); > + > +#endif IMO, a single line of function declaration can be merged to sound/pcm.h. > diff --git a/sound/core/Kconfig b/sound/core/Kconfig > index 313f22e9d929..b534c8a6046b 100644 > --- a/sound/core/Kconfig > +++ b/sound/core/Kconfig > @@ -6,6 +6,9 @@ config SND_PCM > tristate > select SND_TIMER > > +config SND_PCM_ELD > + bool I don't mind much adding this one, but a new Kconfig is always a question. I'd like to hear other's opinion, too. > config SND_DMAENGINE_PCM > tristate > > diff --git a/sound/core/Makefile b/sound/core/Makefile > index 4daf2f58261c..591b49157b4d 100644 > --- a/sound/core/Makefile > +++ b/sound/core/Makefile > @@ -13,6 +13,7 @@ snd-$(CONFIG_SND_JACK) += jack.o > snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_timer.o pcm_misc.o \ > pcm_memory.o memalloc.o > snd-pcm-$(CONFIG_SND_DMA_SGBUF) += sgbuf.o > +snd-pcm-$(CONFIG_SND_PCM_ELD) += pcm_drm_eld.o > > # for trace-points > CFLAGS_pcm_lib.o := -I$(src) > diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c > new file mode 100644 > index 000000000000..47d9b60435fe > --- /dev/null > +++ b/sound/core/pcm_drm_eld.c > @@ -0,0 +1,92 @@ > +/* > + * PCM DRM helpers > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <linux/export.h> > +#include <drm/drm_edid.h> > +#include <sound/pcm.h> > +#include <sound/pcm_drm_eld.h> > + > +static const unsigned int eld_rates[] = { > + 32000, > + 44100, > + 48000, > + 88200, > + 96000, > + 176400, > + 192000, > +}; > + > +static int eld_limit_rates(struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) > +{ > + struct snd_interval *r = hw_param_interval(params, rule->var); > + struct snd_interval *c; > + unsigned int rate_mask = 7, i; > + const u8 *sad, *eld = rule->private; > + > + sad = drm_eld_sad(eld); > + if (sad) { > + c = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); > + > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { > + unsigned channels = 1 + (sad[0] & 7); > + > + /* > + * Exclude SADs which do not include the > + * requested number of channels. > + */ > + if (c->min <= channels) > + rate_mask |= sad[1]; > + } > +printk("%s: r %d-%d c %d-%d m %x\n", __func__, r->min, r->max, c->min, c->max, rate_mask); I guess this will be removed in the final version? ;) > + } > + > + return snd_interval_list(r, ARRAY_SIZE(eld_rates), eld_rates, > + rate_mask); > +} > + > +static int eld_limit_channels(struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) > +{ > + struct snd_interval *var = hw_param_interval(params, rule->var); > + struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; > + unsigned int i; > + const u8 *sad, *eld = rule->private; > + > + sad = drm_eld_sad(eld); > + if (sad) { > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { > + switch (sad[0] & 0x78) { > + case 0x08: > + t.max = max(t.max, (sad[0] & 7) + 1u); > + break; What about the minimal channel? Ideally, we should either give a list of channel numbers or process the hw_constraints dynamically to narrow the min/max matching with the eld. thanks, Takashi > + } > + } > + } > + > + return snd_interval_refine(var, &t); > +} > + > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld) > +{ > + int ret; > + > + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, > + eld_limit_rates, eld, > + SNDRV_PCM_HW_PARAM_RATE, > + SNDRV_PCM_HW_PARAM_CHANNELS, -1); > + if (ret < 0) > + return ret; > + > + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, > + eld_limit_channels, eld, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + SNDRV_PCM_HW_PARAM_RATE, -1); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(snd_pcm_hw_constraint_eld); > -- > 1.8.3.1 >
On Sun, Apr 05, 2015 at 05:57:09PM +0200, Takashi Iwai wrote: > > diff --git a/include/sound/pcm_drm_eld.h b/include/sound/pcm_drm_eld.h > > new file mode 100644 > > index 000000000000..93357b25d2e2 > > --- /dev/null > > +++ b/include/sound/pcm_drm_eld.h > > @@ -0,0 +1,6 @@ > > +#ifndef __SOUND_PCM_DRM_ELD_H > > +#define __SOUND_PCM_DRM_ELD_H > > + > > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld); > > + > > +#endif > > IMO, a single line of function declaration can be merged to > sound/pcm.h. Ok. > > diff --git a/sound/core/Kconfig b/sound/core/Kconfig > > index 313f22e9d929..b534c8a6046b 100644 > > --- a/sound/core/Kconfig > > +++ b/sound/core/Kconfig > > @@ -6,6 +6,9 @@ config SND_PCM > > tristate > > select SND_TIMER > > > > +config SND_PCM_ELD > > + bool > > I don't mind much adding this one, but a new Kconfig is always a > question. I'd like to hear other's opinion, too. That's more a question whether you want it always included in the build or not, especially as it is dependent on DRM header files. > > +printk("%s: r %d-%d c %d-%d m %x\n", __func__, r->min, r->max, c->min, c->max, rate_mask); > > I guess this will be removed in the final version? ;) Of course. > > +static int eld_limit_channels(struct snd_pcm_hw_params *params, > > + struct snd_pcm_hw_rule *rule) > > +{ > > + struct snd_interval *var = hw_param_interval(params, rule->var); > > + struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; > > + unsigned int i; > > + const u8 *sad, *eld = rule->private; > > + > > + sad = drm_eld_sad(eld); > > + if (sad) { > > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { > > + switch (sad[0] & 0x78) { > > + case 0x08: > > + t.max = max(t.max, (sad[0] & 7) + 1u); > > + break; > > What about the minimal channel? There isn't a minimum. The SAD describes only the _maximum_ number of channels. For example, if a TV supports 5.1 audio, at 32, 44.1 and 48 kHz, it can do that with just one SAD entry which indicates support for six channels of audio at those sample rates. However, it will happily accept 2 channel audio at those sample rates too. > Ideally, we should either give a list of channel numbers or process > the hw_constraints dynamically to narrow the min/max matching with the > eld. The ELD can change as a result of the HDMI sink deciding to change its EDID (it does happen) or if the device is unplugged and re-plugged. If I wanted to restrict the maximum channel/rates by building some sort of table, I'd do this at open time and avoid the complexity of having rule callbacks. Since (afaik) ALSA has a lack of support for dynamic reconfiguration according to the attached device changing, the best we can do without a huge amount of re-work of HDMI support across all adapters is to process the capabilities of the attached device at prepare time according to the current capabilities. Implementing dynamic reconfiguration in ALSA is not something I want to get involved with, and as we /already/ have HDMI support merged in the kernel, this is not a blocker for at least trying to get some semblence of sanity, rather than having every driver re-implementing stuff like this.
At Sun, 5 Apr 2015 17:20:34 +0100, Russell King - ARM Linux wrote: > > On Sun, Apr 05, 2015 at 05:57:09PM +0200, Takashi Iwai wrote: > > > diff --git a/include/sound/pcm_drm_eld.h b/include/sound/pcm_drm_eld.h > > > new file mode 100644 > > > index 000000000000..93357b25d2e2 > > > --- /dev/null > > > +++ b/include/sound/pcm_drm_eld.h > > > @@ -0,0 +1,6 @@ > > > +#ifndef __SOUND_PCM_DRM_ELD_H > > > +#define __SOUND_PCM_DRM_ELD_H > > > + > > > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld); > > > + > > > +#endif > > > > IMO, a single line of function declaration can be merged to > > sound/pcm.h. > > Ok. > > > > diff --git a/sound/core/Kconfig b/sound/core/Kconfig > > > index 313f22e9d929..b534c8a6046b 100644 > > > --- a/sound/core/Kconfig > > > +++ b/sound/core/Kconfig > > > @@ -6,6 +6,9 @@ config SND_PCM > > > tristate > > > select SND_TIMER > > > > > > +config SND_PCM_ELD > > > + bool > > > > I don't mind much adding this one, but a new Kconfig is always a > > question. I'd like to hear other's opinion, too. > > That's more a question whether you want it always included in the build > or not, especially as it is dependent on DRM header files. OK, then it makes sense to split. > > > +printk("%s: r %d-%d c %d-%d m %x\n", __func__, r->min, r->max, c->min, c->max, rate_mask); > > > > I guess this will be removed in the final version? ;) > > Of course. > > > > +static int eld_limit_channels(struct snd_pcm_hw_params *params, > > > + struct snd_pcm_hw_rule *rule) > > > +{ > > > + struct snd_interval *var = hw_param_interval(params, rule->var); > > > + struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; > > > + unsigned int i; > > > + const u8 *sad, *eld = rule->private; > > > + > > > + sad = drm_eld_sad(eld); > > > + if (sad) { > > > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { > > > + switch (sad[0] & 0x78) { > > > + case 0x08: > > > + t.max = max(t.max, (sad[0] & 7) + 1u); > > > + break; > > > > What about the minimal channel? > > There isn't a minimum. The SAD describes only the _maximum_ number of > channels. For example, if a TV supports 5.1 audio, at 32, 44.1 and 48 > kHz, it can do that with just one SAD entry which indicates support for > six channels of audio at those sample rates. However, it will happily > accept 2 channel audio at those sample rates too. Alright, I remembered wrong. I took a look at the existing HD-audio ELD parser code, and it also handles only the max channels. > > Ideally, we should either give a list of channel numbers or process > > the hw_constraints dynamically to narrow the min/max matching with the > > eld. > > The ELD can change as a result of the HDMI sink deciding to change its > EDID (it does happen) or if the device is unplugged and re-plugged. If > I wanted to restrict the maximum channel/rates by building some sort of > table, I'd do this at open time and avoid the complexity of having rule > callbacks. Right, this is the easiest approach. > Since (afaik) ALSA has a lack of support for dynamic reconfiguration > according to the attached device changing, the best we can do without > a huge amount of re-work of HDMI support across all adapters is to > process the capabilities of the attached device at prepare time > according to the current capabilities. Yeah, reconfiguration is tricky. BTW, how is the HDMI unplug handled during playback? > Implementing dynamic reconfiguration in ALSA is not something I want to > get involved with, and as we /already/ have HDMI support merged in the > kernel, this is not a blocker for at least trying to get some semblence > of sanity, rather than having every driver re-implementing stuff like > this. Well, I didn't mean about the dynamic reconfiguration. I thought of rather min/max pairs, but it was just a wrong assumption. Scratch it. One another question: don't we need to deal with the sample bits in sad[2]? Takashi
On Sun, Apr 05, 2015 at 06:46:13PM +0200, Takashi Iwai wrote: > At Sun, 5 Apr 2015 17:20:34 +0100, > Russell King - ARM Linux wrote: > > Since (afaik) ALSA has a lack of support for dynamic reconfiguration > > according to the attached device changing, the best we can do without > > a huge amount of re-work of HDMI support across all adapters is to > > process the capabilities of the attached device at prepare time > > according to the current capabilities. > > Yeah, reconfiguration is tricky. BTW, how is the HDMI unplug handled > during playback? We don't handle it right now - and we don't have any notification to the audio drivers that that has happened. Even if we did have such a notification, I'm not sure what the audio driver could do with it at the moment. > > Implementing dynamic reconfiguration in ALSA is not something I want to > > get involved with, and as we /already/ have HDMI support merged in the > > kernel, this is not a blocker for at least trying to get some semblence > > of sanity, rather than having every driver re-implementing stuff like > > this. > > Well, I didn't mean about the dynamic reconfiguration. I thought of > rather min/max pairs, but it was just a wrong assumption. Scratch > it. > > One another question: don't we need to deal with the sample bits in > sad[2]? It should, but I'm very wary about doing that without seeing more examples of real SADs. Right now, all my examples only support one SAD with either 2 channel or 6 channel audio at the standard (basic) 32, 44.1 and 48kHz rates. The HDMI / CEA specs are very loose in their wording about the short audio descriptors. I've no idea whether a sink can provide (for example) descriptors such as: LPCM, 6 channel 32, 44.1, 48kHz LPCM, 2 channel, 32, 44.1, 48, 96, 192kHz or whether have to describe that as a single descriptor. I only have two TVs to test with here. What I'm concerned about is that when the ALSA parameter refining starts, we start with (eg) 2-8 channels, 32-192kHz. Given that, if we invoke the channel restriction before the rate restriction, we would end up limiting to 2 channel at 32-192kHz. If we apply the restrictions in the opposite order, we'd restrict to 6 channel, 32-48kHz. Neither are obviously correct in this circumstance, and I don't really see a way to solve it given my understanding of the way ALSA's parameter refinement works. I suspect this is why most HDMI drivers are implemented such that they take the maximum capabilities over all SADs, which would end up restricting audio in the above case to: up to 6 channels, at 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't hasn't been indicated as supported. Most of this is speculation though, based off what is in the documentation. As I say, I don't have enough real-world examples to get a feel for what manufacturers _actually_ do to give a hint as to how the documentation should be interpreted. So, maybe I should just copy what everyone else does and take the maximum of all descriptors...
On Sun, Apr 05, 2015 at 05:57:09PM +0200, Takashi Iwai wrote: > At Thu, 02 Apr 2015 10:22:06 +0100, > Russell King wrote: > > +config SND_PCM_ELD > > + bool > I don't mind much adding this one, but a new Kconfig is always a > question. I'd like to hear other's opinion, too. One point in favour of this is that there's been some discussion recently of IoT applications with very low resource requirements that might need audion functionality so it's likely that people will be taking a look at trying to minimize code size for the audio stack. This sort of configurability may well be useful for such applications.
On Tue, 2015-05-05 at 23:35 +0100, Mark Brown wrote: > On Sun, Apr 05, 2015 at 05:57:09PM +0200, Takashi Iwai wrote: > > At Thu, 02 Apr 2015 10:22:06 +0100, > > Russell King wrote: > > > > +config SND_PCM_ELD > > > + bool > > > I don't mind much adding this one, but a new Kconfig is always a > > question. I'd like to hear other's opinion, too. > > One point in favour of this is that there's been some discussion > recently of IoT applications with very low resource requirements that > might need audion functionality so it's likely that people will be > taking a look at trying to minimize code size for the audio stack. This > sort of configurability may well be useful for such applications. Fwiw, Keyon is looking at reducing runtime audio DRAM usage atm and will probably have to add other Kconfig options to the audio kernel config e.g. disable DPCM, disable DAPM, disable HW/SW params refinement, disable async audio ioctls, etc. Currently the kernel audio core and drivers are coming in at between 300k to 500k, which is a lot for a device with limited memory and very simple audio requirements. Liam
05.04.2015, 20:26, Russell King - ARM Linux kirjoitti: > On Sun, Apr 05, 2015 at 06:46:13PM +0200, Takashi Iwai wrote: >> >> One another question: don't we need to deal with the sample bits in >> sad[2]? > > It should, but I'm very wary about doing that without seeing more > examples of real SADs. Right now, all my examples only support > one SAD with either 2 channel or 6 channel audio at the standard > (basic) 32, 44.1 and 48kHz rates. > > The HDMI / CEA specs are very loose in their wording about the > short audio descriptors. I've no idea whether a sink can provide > (for example) descriptors such as: > > LPCM, 6 channel 32, 44.1, 48kHz > LPCM, 2 channel, 32, 44.1, 48, 96, 192kHz > > or whether have to describe that as a single descriptor. I only > have two TVs to test with here. For the record, yes, multiple LPCM SADs are relatively common, and I think I've seen some that offered more rates on a stereo SAD than on a multichannel SAD (like in your example). > What I'm concerned about is that when the ALSA parameter refining > starts, we start with (eg) 2-8 channels, 32-192kHz. Given that, > if we invoke the channel restriction before the rate restriction, > we would end up limiting to 2 channel at 32-192kHz. If we apply > the restrictions in the opposite order, we'd restrict to 6 > channel, 32-48kHz. Neither are obviously correct in this > circumstance, and I don't really see a way to solve it given my > understanding of the way ALSA's parameter refinement works. > > I suspect this is why most HDMI drivers are implemented such that > they take the maximum capabilities over all SADs, which would end > up restricting audio in the above case to: up to 6 channels, at > 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't > hasn't been indicated as supported. > > Most of this is speculation though, based off what is in the > documentation. As I say, I don't have enough real-world examples > to get a feel for what manufacturers _actually_ do to give a hint > as to how the documentation should be interpreted. > > So, maybe I should just copy what everyone else does and take the > maximum of all descriptors... >
On Wed, May 06, 2015 at 08:02:52PM +0300, Anssi Hannula wrote: > 05.04.2015, 20:26, Russell King - ARM Linux kirjoitti: > > On Sun, Apr 05, 2015 at 06:46:13PM +0200, Takashi Iwai wrote: > >> > >> One another question: don't we need to deal with the sample bits in > >> sad[2]? > > > > It should, but I'm very wary about doing that without seeing more > > examples of real SADs. Right now, all my examples only support > > one SAD with either 2 channel or 6 channel audio at the standard > > (basic) 32, 44.1 and 48kHz rates. > > > > The HDMI / CEA specs are very loose in their wording about the > > short audio descriptors. I've no idea whether a sink can provide > > (for example) descriptors such as: > > > > LPCM, 6 channel 32, 44.1, 48kHz > > LPCM, 2 channel, 32, 44.1, 48, 96, 192kHz > > > > or whether have to describe that as a single descriptor. I only > > have two TVs to test with here. > > For the record, yes, multiple LPCM SADs are relatively common, and I > think I've seen some that offered more rates on a stereo SAD than on a > multichannel SAD (like in your example). Okay, so we "think" we've seen it in the wild... > > What I'm concerned about is that when the ALSA parameter refining > > starts, we start with (eg) 2-8 channels, 32-192kHz. Given that, > > if we invoke the channel restriction before the rate restriction, > > we would end up limiting to 2 channel at 32-192kHz. If we apply > > the restrictions in the opposite order, we'd restrict to 6 > > channel, 32-48kHz. Neither are obviously correct in this > > circumstance, and I don't really see a way to solve it given my > > understanding of the way ALSA's parameter refinement works. > > > > I suspect this is why most HDMI drivers are implemented such that > > they take the maximum capabilities over all SADs, which would end > > up restricting audio in the above case to: up to 6 channels, at > > 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't > > hasn't been indicated as supported. > > > > Most of this is speculation though, based off what is in the > > documentation. As I say, I don't have enough real-world examples > > to get a feel for what manufacturers _actually_ do to give a hint > > as to how the documentation should be interpreted. ... so this is probably less than speculation. So, ALSA gurus, how do we handle this? How do we arrange the parameter resolution such that ALSA can permit _either_ 2 channels at 192kHz or 6 channels at 48kHz, but not 6 channels at 192kHz? Right now, I don't see how that's possible.
On 05/07/2015 12:41 PM, Russell King - ARM Linux wrote: [...] >>> What I'm concerned about is that when the ALSA parameter refining >>> starts, we start with (eg) 2-8 channels, 32-192kHz. Given that, >>> if we invoke the channel restriction before the rate restriction, >>> we would end up limiting to 2 channel at 32-192kHz. If we apply >>> the restrictions in the opposite order, we'd restrict to 6 >>> channel, 32-48kHz. Neither are obviously correct in this >>> circumstance, and I don't really see a way to solve it given my >>> understanding of the way ALSA's parameter refinement works. >>> >>> I suspect this is why most HDMI drivers are implemented such that >>> they take the maximum capabilities over all SADs, which would end >>> up restricting audio in the above case to: up to 6 channels, at >>> 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't >>> hasn't been indicated as supported. >>> >>> Most of this is speculation though, based off what is in the >>> documentation. As I say, I don't have enough real-world examples >>> to get a feel for what manufacturers _actually_ do to give a hint >>> as to how the documentation should be interpreted. > > ... so this is probably less than speculation. > > So, ALSA gurus, how do we handle this? How do we arrange the parameter > resolution such that ALSA can permit _either_ 2 channels at 192kHz or 6 > channels at 48kHz, but not 6 channels at 192kHz? Right now, I don't > see how that's possible. That's pretty straight forward and can be done using custom rules linking the number of channels to the sample rate. Have a look at snd_ac97_pcm_double_rate_rules() this is pretty much the same constraint. - Lars
On 2015-04-05 20:26, Russell King - ARM Linux wrote: > On Sun, Apr 05, 2015 at 06:46:13PM +0200, Takashi Iwai wrote: >> At Sun, 5 Apr 2015 17:20:34 +0100, >> Russell King - ARM Linux wrote: >> > Since (afaik) ALSA has a lack of support for dynamic reconfiguration >> > according to the attached device changing, the best we can do without >> > a huge amount of re-work of HDMI support across all adapters is to >> > process the capabilities of the attached device at prepare time >> > according to the current capabilities. >> >> Yeah, reconfiguration is tricky. BTW, how is the HDMI unplug handled >> during playback? > > We don't handle it right now - and we don't have any notification to > the audio drivers that that has happened. Even if we did have such a > notification, I'm not sure what the audio driver could do with it at > the moment. > >> > Implementing dynamic reconfiguration in ALSA is not something I want to >> > get involved with, and as we /already/ have HDMI support merged in the >> > kernel, this is not a blocker for at least trying to get some semblence >> > of sanity, rather than having every driver re-implementing stuff like >> > this. >> >> Well, I didn't mean about the dynamic reconfiguration. I thought of >> rather min/max pairs, but it was just a wrong assumption. Scratch >> it. >> >> One another question: don't we need to deal with the sample bits in >> sad[2]? > > It should, but I'm very wary about doing that without seeing more > examples of real SADs. If the same constraint setting helpers are to be used also with external HDMI encoders with i2s interface, there should be an option to leave out the constraints for the sample bits. There is no direct connection between the number of bits on I2S and HDMI link. The CPU DAI may apply its own constraints on the available sample formats and too strict constraints at the encoder end may lead to zero available formats. Best regrads, Jyri > Right now, all my examples only support > one SAD with either 2 channel or 6 channel audio at the standard > (basic) 32, 44.1 and 48kHz rates. > > The HDMI / CEA specs are very loose in their wording about the > short audio descriptors. I've no idea whether a sink can provide > (for example) descriptors such as: > > LPCM, 6 channel 32, 44.1, 48kHz > LPCM, 2 channel, 32, 44.1, 48, 96, 192kHz > > or whether have to describe that as a single descriptor. I only > have two TVs to test with here. > > What I'm concerned about is that when the ALSA parameter refining > starts, we start with (eg) 2-8 channels, 32-192kHz. Given that, > if we invoke the channel restriction before the rate restriction, > we would end up limiting to 2 channel at 32-192kHz. If we apply > the restrictions in the opposite order, we'd restrict to 6 > channel, 32-48kHz. Neither are obviously correct in this > circumstance, and I don't really see a way to solve it given my > understanding of the way ALSA's parameter refinement works. > > I suspect this is why most HDMI drivers are implemented such that > they take the maximum capabilities over all SADs, which would end > up restricting audio in the above case to: up to 6 channels, at > 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't > hasn't been indicated as supported. > > Most of this is speculation though, based off what is in the > documentation. As I say, I don't have enough real-world examples > to get a feel for what manufacturers _actually_ do to give a hint > as to how the documentation should be interpreted. > > So, maybe I should just copy what everyone else does and take the > maximum of all descriptors...
On Fri, May 08, 2015 at 01:56:02PM +0300, Jyri Sarha wrote: > On 2015-04-05 20:26, Russell King - ARM Linux wrote: > >On Sun, Apr 05, 2015 at 06:46:13PM +0200, Takashi Iwai wrote: > >>At Sun, 5 Apr 2015 17:20:34 +0100, > >>Russell King - ARM Linux wrote: > >>> Since (afaik) ALSA has a lack of support for dynamic reconfiguration > >>> according to the attached device changing, the best we can do without > >>> a huge amount of re-work of HDMI support across all adapters is to > >>> process the capabilities of the attached device at prepare time > >>> according to the current capabilities. > >> > >>Yeah, reconfiguration is tricky. BTW, how is the HDMI unplug handled > >>during playback? > > > >We don't handle it right now - and we don't have any notification to > >the audio drivers that that has happened. Even if we did have such a > >notification, I'm not sure what the audio driver could do with it at > >the moment. > > > >>> Implementing dynamic reconfiguration in ALSA is not something I want to > >>> get involved with, and as we /already/ have HDMI support merged in the > >>> kernel, this is not a blocker for at least trying to get some semblence > >>> of sanity, rather than having every driver re-implementing stuff like > >>> this. > >> > >>Well, I didn't mean about the dynamic reconfiguration. I thought of > >>rather min/max pairs, but it was just a wrong assumption. Scratch > >>it. > >> > >>One another question: don't we need to deal with the sample bits in > >>sad[2]? > > > >It should, but I'm very wary about doing that without seeing more > >examples of real SADs. > > If the same constraint setting helpers are to be used also with > external HDMI encoders with i2s interface, there should be an option > to leave out the constraints for the sample bits. There is no direct > connection between the number of bits on I2S and HDMI link. The CPU > DAI may apply its own constraints on the available sample formats and > too strict constraints at the encoder end may lead to zero available > formats. Possibly, but then how do we know which SAD to apply to limit the number of channels? I suspect for the use case you're suggesting above, it's better left to the user rather than generic code to work it out otherwise things get far too complex. We'd need to have some way for the driver to report back to this generic code the actual number of bits on the HDMI link. However, as we currently know, ALSA appears to have a problem here which pretty much makes properly restricting the channels and sample rates impossible to do. That issue is the one I'm much more keen to solve right now, because without that being solved, this set of patches is pretty much dead in the water.
On 2015-04-02 12:22, Russell King wrote: > Add a helper for the EDID like data structure, which is typically > passed > from a HDMI adapter to its associated audio driver. This informs the > audio driver of the capabilities of the attached HDMI sink. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > include/sound/pcm_drm_eld.h | 6 +++ > sound/core/Kconfig | 3 ++ > sound/core/Makefile | 1 + > sound/core/pcm_drm_eld.c | 92 > +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 102 insertions(+) > create mode 100644 include/sound/pcm_drm_eld.h > create mode 100644 sound/core/pcm_drm_eld.c > > diff --git a/include/sound/pcm_drm_eld.h b/include/sound/pcm_drm_eld.h > new file mode 100644 > index 000000000000..93357b25d2e2 > --- /dev/null > +++ b/include/sound/pcm_drm_eld.h > @@ -0,0 +1,6 @@ > +#ifndef __SOUND_PCM_DRM_ELD_H > +#define __SOUND_PCM_DRM_ELD_H > + > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void > *eld); > + > +#endif > diff --git a/sound/core/Kconfig b/sound/core/Kconfig > index 313f22e9d929..b534c8a6046b 100644 > --- a/sound/core/Kconfig > +++ b/sound/core/Kconfig > @@ -6,6 +6,9 @@ config SND_PCM > tristate > select SND_TIMER > > +config SND_PCM_ELD > + bool > + > config SND_DMAENGINE_PCM > tristate > > diff --git a/sound/core/Makefile b/sound/core/Makefile > index 4daf2f58261c..591b49157b4d 100644 > --- a/sound/core/Makefile > +++ b/sound/core/Makefile > @@ -13,6 +13,7 @@ snd-$(CONFIG_SND_JACK) += jack.o > snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_timer.o pcm_misc.o \ > pcm_memory.o memalloc.o > snd-pcm-$(CONFIG_SND_DMA_SGBUF) += sgbuf.o > +snd-pcm-$(CONFIG_SND_PCM_ELD) += pcm_drm_eld.o > > # for trace-points > CFLAGS_pcm_lib.o := -I$(src) > diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c > new file mode 100644 > index 000000000000..47d9b60435fe > --- /dev/null > +++ b/sound/core/pcm_drm_eld.c > @@ -0,0 +1,92 @@ > +/* > + * PCM DRM helpers > + * > + * This program is free software; you can redistribute it and/or > modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <linux/export.h> > +#include <drm/drm_edid.h> > +#include <sound/pcm.h> > +#include <sound/pcm_drm_eld.h> > + > +static const unsigned int eld_rates[] = { > + 32000, > + 44100, > + 48000, > + 88200, > + 96000, > + 176400, > + 192000, > +}; > + For what I can see, you are setting the cross dependency between the sample rate and channels incorrectly. You should look for the currently tested channel count directly from hw params. > +static int eld_limit_rates(struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) > +{ > + struct snd_interval *r = hw_param_interval(params, rule->var); > + struct snd_interval *c; > + unsigned int rate_mask = 7, i; > + const u8 *sad, *eld = rule->private; > + > + sad = drm_eld_sad(eld); > + if (sad) { > + c = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - c = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + uint pchannels = params_channels(params); > + > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { > + unsigned channels = 1 + (sad[0] & 7); > + > + /* > + * Exclude SADs which do not include the > + * requested number of channels. > + */ > + if (pchannels <= channels) > + rate_mask |= sad[1]; > + } > +printk("%s: r %d-%d c %d-%d m %x\n", __func__, r->min, r->max, > c->min, c->max, rate_mask); > + } > + > + return snd_interval_list(r, ARRAY_SIZE(eld_rates), eld_rates, > + rate_mask); > +} > + From this side the dependency is missing all together. > +static int eld_limit_channels(struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) > +{ > + struct snd_interval *var = hw_param_interval(params, rule->var); > + struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; > + unsigned int i; + unsigned int j; > + const u8 *sad, *eld = rule->private; + int rate = params_rate(params); > + > + sad = drm_eld_sad(eld); > + if (sad) { > + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { + for (j = 0; j < ARRAY_SIZE(eld_rates); j++) + if ((sad[1] & (1<<j)) && rate == eld_rates[j]) { > + switch (sad[0] & 0x78) { > + case 0x08: > + t.max = max(t.max, (sad[0] & 7) + 1u); > + break; > + } + } > + } > + } > + > + return snd_interval_refine(var, &t); > +} > + The code is completely untested, but I am sure you got the idea. Best regards, Jyri > +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void > *eld) > +{ > + int ret; > + > + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, > + eld_limit_rates, eld, > + SNDRV_PCM_HW_PARAM_RATE, > + SNDRV_PCM_HW_PARAM_CHANNELS, -1); > + if (ret < 0) > + return ret; > + > + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, > + eld_limit_channels, eld, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + SNDRV_PCM_HW_PARAM_RATE, -1); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(snd_pcm_hw_constraint_eld);
On Fri, May 08, 2015 at 04:16:08PM +0300, Jyri Sarha wrote: > On 2015-04-02 12:22, Russell King wrote: > >+static const unsigned int eld_rates[] = { > >+ 32000, > >+ 44100, > >+ 48000, > >+ 88200, > >+ 96000, > >+ 176400, > >+ 192000, > >+}; > >+ > > For what I can see, you are setting the cross dependency between > the sample rate and channels incorrectly. There is a dependency between sample rate and channels. > You should look for the > currently tested channel count directly from hw params. I'm not so sure that's right after looking at snd_ac97_pcm_double_rate_rules(). That's pretty much the same problem - AC'97 codecs can operate at a sample rate of either the bus frame frequency or twice the bus frame frequency. They achieve twice the bus frame frequency by reallocating a couple of the PCM data slots which would otherwise be used for >2 channels to the first two channels, thus allowing two front channel samples per frame. AC'97 limits the number of channels to two if: if (rate->min > 48000) { and limits the sample rate to 1-48kHz if: if (channels->min > 2) { > From this side the dependency is missing all together. Yes, it I initially couldn't work out how to do that bit... but I have a solution now which seems to sort-of work. Testing with aplay, I find that it seems to mostly work, and I guess that: aplay -D hw:0,0 -v -f S24_LE -c 6 -r 192000 /dev/zero resulting in: Playing raw data '/dev/zero' : Signed 24 bit Little Endian, Rate 192000 Hz, Channels 6 Warning: rate is not accurate (requested = 192000Hz, got = 48000Hz) please, try the plug plugin Hardware PCM card 0 'DW-HDMI' device 0 subdevice 0 Its setup is: stream : PLAYBACK access : RW_INTERLEAVED format : S24_LE subformat : STD channels : 6 rate : 48000 exact rate : 48000 (48000/1) is acceptable. Also looking at AC'97, I don't need to list the same parameter as a dependent of the rule... something I'll try when I'm back from the hospital later this afternoon...
On 05/08/15 16:27, Russell King - ARM Linux wrote: > On Fri, May 08, 2015 at 04:16:08PM +0300, Jyri Sarha wrote: >> On 2015-04-02 12:22, Russell King wrote: ... >> For what I can see, you are setting the cross dependency between >> the sample rate and channels incorrectly. > > There is a dependency between sample rate and channels. > >> You should look for the >> currently tested channel count directly from hw params. > > I'm not so sure that's right after looking at snd_ac97_pcm_double_rate_rules(). > That's pretty much the same problem - AC'97 codecs can operate at a > sample rate of either the bus frame frequency or twice the bus frame > frequency. > Maybe digging the channels from intervals works too, I have just never used that myself. The param_channels() works too and it is slightly simpler.
diff --git a/include/sound/pcm_drm_eld.h b/include/sound/pcm_drm_eld.h new file mode 100644 index 000000000000..93357b25d2e2 --- /dev/null +++ b/include/sound/pcm_drm_eld.h @@ -0,0 +1,6 @@ +#ifndef __SOUND_PCM_DRM_ELD_H +#define __SOUND_PCM_DRM_ELD_H + +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld); + +#endif diff --git a/sound/core/Kconfig b/sound/core/Kconfig index 313f22e9d929..b534c8a6046b 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -6,6 +6,9 @@ config SND_PCM tristate select SND_TIMER +config SND_PCM_ELD + bool + config SND_DMAENGINE_PCM tristate diff --git a/sound/core/Makefile b/sound/core/Makefile index 4daf2f58261c..591b49157b4d 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -13,6 +13,7 @@ snd-$(CONFIG_SND_JACK) += jack.o snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_timer.o pcm_misc.o \ pcm_memory.o memalloc.o snd-pcm-$(CONFIG_SND_DMA_SGBUF) += sgbuf.o +snd-pcm-$(CONFIG_SND_PCM_ELD) += pcm_drm_eld.o # for trace-points CFLAGS_pcm_lib.o := -I$(src) diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c new file mode 100644 index 000000000000..47d9b60435fe --- /dev/null +++ b/sound/core/pcm_drm_eld.c @@ -0,0 +1,92 @@ +/* + * PCM DRM helpers + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/export.h> +#include <drm/drm_edid.h> +#include <sound/pcm.h> +#include <sound/pcm_drm_eld.h> + +static const unsigned int eld_rates[] = { + 32000, + 44100, + 48000, + 88200, + 96000, + 176400, + 192000, +}; + +static int eld_limit_rates(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_interval *r = hw_param_interval(params, rule->var); + struct snd_interval *c; + unsigned int rate_mask = 7, i; + const u8 *sad, *eld = rule->private; + + sad = drm_eld_sad(eld); + if (sad) { + c = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { + unsigned channels = 1 + (sad[0] & 7); + + /* + * Exclude SADs which do not include the + * requested number of channels. + */ + if (c->min <= channels) + rate_mask |= sad[1]; + } +printk("%s: r %d-%d c %d-%d m %x\n", __func__, r->min, r->max, c->min, c->max, rate_mask); + } + + return snd_interval_list(r, ARRAY_SIZE(eld_rates), eld_rates, + rate_mask); +} + +static int eld_limit_channels(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_interval *var = hw_param_interval(params, rule->var); + struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; + unsigned int i; + const u8 *sad, *eld = rule->private; + + sad = drm_eld_sad(eld); + if (sad) { + for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { + switch (sad[0] & 0x78) { + case 0x08: + t.max = max(t.max, (sad[0] & 7) + 1u); + break; + } + } + } + + return snd_interval_refine(var, &t); +} + +int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld) +{ + int ret; + + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, + eld_limit_rates, eld, + SNDRV_PCM_HW_PARAM_RATE, + SNDRV_PCM_HW_PARAM_CHANNELS, -1); + if (ret < 0) + return ret; + + ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, + eld_limit_channels, eld, + SNDRV_PCM_HW_PARAM_CHANNELS, + SNDRV_PCM_HW_PARAM_RATE, -1); + + return ret; +} +EXPORT_SYMBOL_GPL(snd_pcm_hw_constraint_eld);
Add a helper for the EDID like data structure, which is typically passed from a HDMI adapter to its associated audio driver. This informs the audio driver of the capabilities of the attached HDMI sink. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- include/sound/pcm_drm_eld.h | 6 +++ sound/core/Kconfig | 3 ++ sound/core/Makefile | 1 + sound/core/pcm_drm_eld.c | 92 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+) create mode 100644 include/sound/pcm_drm_eld.h create mode 100644 sound/core/pcm_drm_eld.c