[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Jyri Sarha]

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.

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Jyri Sarha]

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 
> ---
>  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 ..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 ..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 
> +#include 
> +#include 
> +#include 
> +
> +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< +  switch (sad[0] & 0x78) {
> +  case 0x08:
> + t.max = max(t.max, (sad[0] & 7) + 1u);
> + break;
> + }

+   }

> + }
> + }
> +
> + return snd_interval_refine(var, );
> +}
> +

The 

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Russell King - ARM Linux]

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...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Jyri Sarha]

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...

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Lars-Peter Clausen]

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

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Liam Girdwood]

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