dw_hdmi cleanups, audio preparation, helpers and ahb audio support

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 <[email protected]>

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

include/sound/pcm_drm_eld.h new file +0 -0

new file mode 100644
index 000000000000..93357b25d2e2

sound/core/Kconfig +0 -0

 config SND_DMAENGINE_PCM
 	tristate
 

sound/core/Makefile +1 -0

index 4daf2f58261c..591b49157b4d 100644
@@ -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)

sound/core/pcm_drm_eld.c new file +35 -0

new file mode 100644
index 000000000000..47d9b60435fe
@@ -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) {

sound/core/pcm_drm_eld.c new file +11 -0

+	}
+
+	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, };

sound/core/pcm_drm_eld.c new file +26 -0

+			}
+		}
+	}
+
+	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);

Signature · 1 line

1.8.3.1

Quoted 7 lines

include/sound/pcm_drm_eld.h new file +1 -0

new file mode 100644
index 000000000000..93357b25d2e2
@@ -0,0 +1,6 @@
+#ifndef __SOUND_PCM_DRM_ELD_H

include/sound/pcm_drm_eld.h line 6

Quoted 5 lines

include/sound/pcm_drm_eld.h new file +5 -0

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

Quoted 8 lines

sound/core/Kconfig +0 -0

index 313f22e9d929..b534c8a6046b 100644
@@ -6,6 +6,9 @@ config SND_PCM
 	tristate
 	select SND_TIMER
 

sound/core/Kconfig config SND_PCM · line 10

Quoted 2 lines

sound/core/Kconfig +2 -0

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

sound/core/Kconfig config SND_PCM · line 11

Quoted 1 line

sound/core/Kconfig +1 -0

+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? ;)

Quoted 9 lines

sound/core/Kconfig +9 -0

+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);

sound/core/Kconfig

Quoted 6 lines

sound/core/Kconfig +6 -0

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

Quoted 7 lines · Takashi Iwai

Quoted 7 lines

include/sound/pcm_drm_eld.h new file +1 -0

new file mode 100644
index 000000000000..93357b25d2e2
@@ -0,0 +1,6 @@
+#ifndef __SOUND_PCM_DRM_ELD_H

Quoted 8 lines

include/sound/pcm_drm_eld.h line 6

Quoted 5 lines

include/sound/pcm_drm_eld.h new file +5 -0

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

Quoted 8 lines

Quoted 8 lines

sound/core/Kconfig +0 -0

index 313f22e9d929..b534c8a6046b 100644
@@ -6,6 +6,9 @@ config SND_PCM
 	tristate
 	select SND_TIMER
 

Quoted 5 lines

sound/core/Kconfig config SND_PCM · line 10

Quoted 2 lines

sound/core/Kconfig +2 -0

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

Quoted 3 lines

sound/core/Kconfig config SND_PCM · line 11

Quoted 1 line

sound/core/Kconfig +1 -0

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

Quoted 9 lines

Quoted 9 lines

sound/core/Kconfig +9 -0

+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);

Quoted 8 lines

sound/core/Kconfig

Quoted 6 lines

sound/core/Kconfig +6 -0

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

Quoted 3 lines

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,

Quoted 5 lines · Russell King - ARM Linux

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?

Quoted 5 lines

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]?

Quoted 3 lines

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

05.04.2015, 20:26, Russell King - ARM Linux kirjoitti:

Quoted 17 lines · Takashi Iwai

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 15 lines

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.

Quoted 4 lines

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.

Quoted 15 lines

Quoted 15 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 10 lines · Takashi Iwai

At Sun, 5 Apr 2015 17:20:34 +0100,

Quoted 5 lines · Russell King - ARM Linux

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 27 lines · Russell King - ARM Linux

Quoted 10 lines · Takashi Iwai

At Sun, 5 Apr 2015 17:20:34 +0100,

Quoted 5 lines · Russell King - ARM Linux

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

At Thu, 02 Apr 2015 10:22:06 +0100,

sound/core/Kconfig config SND_PCM · line 10

Quoted 2 lines · Russell King

sound/core/Kconfig +2 -0

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

Quoted 8 lines · Takashi Iwai

At Thu, 02 Apr 2015 10:22:06 +0100,

sound/core/Kconfig config SND_PCM · line 10

Quoted 2 lines · Russell King

sound/core/Kconfig +2 -0

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

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 <[email protected]>

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

include/sound/pcm_drm_eld.h new file +4 -0

new file mode 100644
index 000000000000..93357b25d2e2
@@ -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);

include/sound/pcm_drm_eld.h new file +2 -0

+
+#endif

sound/core/Kconfig +3 -0

index 313f22e9d929..b534c8a6046b 100644
@@ -6,6 +6,9 @@ config SND_PCM
 	tristate
 	select SND_TIMER
 
+config SND_PCM_ELD
+	bool
+
 config SND_DMAENGINE_PCM
 	tristate
 

sound/core/Makefile +1 -0

index 4daf2f58261c..591b49157b4d 100644
@@ -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)

sound/core/pcm_drm_eld.c new file +4 -0

new file mode 100644
index 000000000000..47d9b60435fe
@@ -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.
+ */

sound/core/pcm_drm_eld.c new file +5 -0

+#include <linux/export.h>
+#include <drm/drm_edid.h>
+#include <sound/pcm.h>
+#include <sound/pcm_drm_eld.h>
+

sound/core/pcm_drm_eld.c new file +12 -0

+
+		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);

+int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld)

sound/core/pcm_drm_eld.c new file +18 -0

+{
+	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);

sound/core/pcm_drm_eld.c line 22

Quoted 10 lines · Russell King

sound/core/pcm_drm_eld.c new file +10 -0

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

From this side the dependency is missing all together.

Quoted 1 line · Jyri Sarha

On 2015-04-02 12:22, Russell King wrote:

Quoted 2 lines

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.

Quoted 2 lines

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.

The phy configuration is dependent on the SoC, and we look up values for some of the registers in SoC specific data. However, we had partially programmed the phy before we had successfully looked up the clock rate. Also, we were only checking that we had a valid configuration for the currctrl register.

Move all these lookups to the start of this function instead, so we can check that all lookups were successful before beginning to program the phy.

Signed-off-by Russell King <[email protected]>

---
  drivers/gpu/drm/bridge/dw_hdmi.c | 68 +++++++++++++++++++++-------------------
  1 file changed, 35 insertions(+), 33 deletions(-)

drivers/gpu/drm/bridge/dw_hdmi.c +32 -29

index 3494391e4199..23ea8c5c85b6 100644
@@ -753,12 +753,12 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
  static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
  			      unsigned char res, int cscon)
  {
-	unsigned res_idx, i;
+	unsigned res_idx;
  	u8 val, msec;
-	const struct dw_hdmi_plat_data *plat_data = hdmi->plat_data;
-	const struct dw_hdmi_mpll_config *mpll_config = plat_data->mpll_cfg;
-	const struct dw_hdmi_curr_ctrl *curr_ctrl = plat_data->cur_ctr;
-	const struct dw_hdmi_phy_config *phy_config = plat_data->phy_config;
+	const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
+	const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
+	const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
+	const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
 
  	if (prep)
  		return -EINVAL;

@@ -778,6 +778,30 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
  		return -EINVAL;
  	}
 
+	/* PLL/MPLL Cfg - always match on final entry */
+	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
+		if (hdmi->hdmi_data.video_mode.mpixelclock <=
+		    mpll_config->mpixelclock)
+			break;
+
+	for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
+		if (hdmi->hdmi_data.video_mode.mpixelclock <=
+		    curr_ctrl->mpixelclock)
+			break;
+
+	for (; phy_config->mpixelclock != ~0UL; phy_config++)
+		if (hdmi->hdmi_data.video_mode.mpixelclock <=
+		    phy_config->mpixelclock)
+			break;
+
+	if (mpll_config->mpixelclock == ~0UL ||
+	    curr_ctrl->mpixelclock == ~0UL ||
+	    phy_config->mpixelclock == ~0UL) {
+		dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
+			hdmi->hdmi_data.video_mode.mpixelclock);
+		return -EINVAL;
+	}
+
  	/* Enable csc path */
  	if (cscon)
  		val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH;

@@ -803,40 +827,18 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
  		    HDMI_PHY_I2CM_SLAVE_ADDR);
  	hdmi_phy_test_clear(hdmi, 0);
 
-	/* PLL/MPLL Cfg - always match on final entry */
-	for (i = 0; mpll_config[i].mpixelclock != (~0UL); i++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    mpll_config[i].mpixelclock)
-			break;
-
-	hdmi_phy_i2c_write(hdmi, mpll_config[i].res[res_idx].cpce, 0x06);
-	hdmi_phy_i2c_write(hdmi, mpll_config[i].res[res_idx].gmp, 0x15);
-
-	for (i = 0; curr_ctrl[i].mpixelclock != (~0UL); i++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    curr_ctrl[i].mpixelclock)
-			break;
-
-	if (curr_ctrl[i].mpixelclock == (~0UL)) {
-		dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
-			hdmi->hdmi_data.video_mode.mpixelclock);
-		return -EINVAL;
-	}
+	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].cpce, 0x06);
+	hdmi_phy_i2c_write(hdmi, mpll_config->res[res_idx].gmp, 0x15);
 
  	/* CURRCTRL */
-	hdmi_phy_i2c_write(hdmi, curr_ctrl[i].curr[res_idx], 0x10);
+	hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[res_idx], 0x10);
 
  	hdmi_phy_i2c_write(hdmi, 0x0000, 0x13);  /* PLLPHBYCTRL */
  	hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
 
-	for (i = 0; phy_config[i].mpixelclock != (~0UL); i++)
-		if (hdmi->hdmi_data.video_mode.mpixelclock <=
-		    phy_config[i].mpixelclock)
-			break;

Clean up hdmi_set_clk_regenerator() by allowing it to take the audio sample rate and ratio directly, rather than hiding it inside the function. Raise the unsupported pixel clock/sample rate message from debug to error level as this results in audio not working correctly.

Signed-off-by Russell King <[email protected]>

---
  drivers/gpu/drm/bridge/dw_hdmi.c | 32 +++++++++++++++-----------------
  1 file changed, 15 insertions(+), 17 deletions(-)

drivers/gpu/drm/bridge/dw_hdmi.c +13 -16

index 23ea8c5c85b6..75099b80ca7e 100644
@@ -335,39 +335,37 @@ static unsigned int hdmi_compute_cts(unsigned int freq, unsigned long pixel_clk,
  }
 
  static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
-				     unsigned long pixel_clk)
+	unsigned long pixel_clk, unsigned int sample_rate, unsigned int ratio)
  {
-	unsigned int clk_n, clk_cts;
+	unsigned int n, cts;
 
-	clk_n = hdmi_compute_n(hdmi->sample_rate, pixel_clk,
-			       hdmi->ratio);
-	clk_cts = hdmi_compute_cts(hdmi->sample_rate, pixel_clk,
-				   hdmi->ratio);
-
-	if (!clk_cts) {
-		dev_dbg(hdmi->dev, "%s: pixel clock not supported: %lu\n",
-			__func__, pixel_clk);
-		return;
+	n = hdmi_compute_n(sample_rate, pixel_clk, ratio);
+	cts = hdmi_compute_cts(sample_rate, pixel_clk, ratio);
+	if (!cts) {
+		dev_err(hdmi->dev,
+			"%s: pixel clock/sample rate not supported: %luMHz / %ukHz\n",
+			__func__, pixel_clk, sample_rate);
  	}
 
-	dev_dbg(hdmi->dev, "%s: samplerate=%d  ratio=%d  pixelclk=%lu  N=%d cts=%d\n",
-		__func__, hdmi->sample_rate, hdmi->ratio,
-		pixel_clk, clk_n, clk_cts);
+	dev_dbg(hdmi->dev, "%s: samplerate=%ukHz ratio=%d pixelclk=%luMHz N=%d cts=%d\n",
+		__func__, sample_rate, ratio, pixel_clk, n, cts);
 
-	hdmi_set_cts_n(hdmi, clk_cts, clk_n);
+	hdmi_set_cts_n(hdmi, cts, n);
  }
 
  static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi)
  {
  	mutex_lock(&hdmi->audio_mutex);
-	hdmi_set_clk_regenerator(hdmi, 74250000);
+	hdmi_set_clk_regenerator(hdmi, 74250000, hdmi->sample_rate,
+				 hdmi->ratio);
  	mutex_unlock(&hdmi->audio_mutex);
  }

Introduce dw_hdmi_set_sample_rate(), which allows us to configure the audio sample rate, setting the CTS/N values appropriately.

Signed-off-by Russell King <[email protected]>

---
  drivers/gpu/drm/bridge/dw_hdmi.c | 10 ++++++++++
  include/drm/bridge/dw_hdmi.h     |  5 +++++
  2 files changed, 15 insertions(+)

drivers/gpu/drm/bridge/dw_hdmi.c +10 -0

index 75728ba552d0..0369fab5c695 100644
@@ -366,6 +366,16 @@ static void hdmi_clk_regenerator_update_pixel_clock(struct dw_hdmi *hdmi)
  	mutex_unlock(&hdmi->audio_mutex);
  }
 
+void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
+{
+	mutex_lock(&hdmi->audio_mutex);
+	hdmi->sample_rate = rate;
+	hdmi_set_clk_regenerator(hdmi, hdmi->hdmi_data.video_mode.mpixelclock,
+				 hdmi->sample_rate, hdmi->ratio);
+	mutex_unlock(&hdmi->audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+
  /*
   * this submodule is responsible for the video data synchronization.
   * for example, for RGB 4:4:4 input, the data map is defined as

include/drm/bridge/dw_hdmi.h +2 -0

index de13bfc35634..763af51e1d60 100644
@@ -12,6 +12,8 @@
 
  #include <drm/drmP.h>
 
+struct dw_hdmi;
+
  enum {
  	DW_HDMI_RES_8,
  	DW_HDMI_RES_10,

iMX6 devices suffer from an errata (ERR005174) where the audio FIFO can be emptied while it is partially full, resulting in misalignment of the audio samples.

To prevent this, the errata workaround recommends writing N as zero until the audio FIFO has been loaded by DMA. Writing N=0 prevents the HDMI bridge from reading from the audio FIFO, effectively disabling audio.

This means we need to provide the audio driver with a pair of functions to enable/disable audio. These are dw_hdmi_audio_enable() and dw_hdmi_audio_disable().

A spinlock is introduced to ensure that setting the CTS/N values can't race, ensuring that the audio driver calling the enable/disable functions (which are called in an atomic context) can't race with a modeset.

Signed-off-by Russell King <[email protected]>

---
  drivers/gpu/drm/bridge/dw_hdmi.c | 34 +++++++++++++++++++++++++++++++++-
  include/drm/bridge/dw_hdmi.h     |  2 ++
  2 files changed, 35 insertions(+), 1 deletion(-)

drivers/gpu/drm/bridge/dw_hdmi.c +33 -1

index 0369fab5c695..adda3a988f36 100644
@@ -18,6 +18,7 @@
  #include <linux/hdmi.h>
  #include <linux/mutex.h>
  #include <linux/of_device.h>
+#include <linux/spinlock.h>
 
  #include <drm/drm_of.h>
  #include <drm/drmP.h>

@@ -124,8 +125,12 @@ struct dw_hdmi {
  	struct i2c_adapter *ddc;
  	void __iomem *regs;
 
+	spinlock_t audio_lock;
  	struct mutex audio_mutex;
  	unsigned int sample_rate;
+	unsigned int audio_cts;
+	unsigned int audio_n;
+	bool audio_enable;
  	int ratio;
 
  	void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);

@@ -347,7 +352,11 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
  	dev_dbg(hdmi->dev, "%s: samplerate=%ukHz ratio=%d pixelclk=%luMHz N=%d cts=%d\n",
  		__func__, sample_rate, ratio, pixel_clk, n, cts);
 
-	hdmi_set_cts_n(hdmi, cts, n);
+	spin_lock_irq(&hdmi->audio_lock);
+	hdmi->audio_n = n;
+	hdmi->audio_cts = cts;
+	hdmi_set_cts_n(hdmi, cts, hdmi->audio_enable ? n : 0);
+	spin_unlock_irq(&hdmi->audio_lock);
  }
 
  static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi)

@@ -376,6 +385,28 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
  }
  EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
 
+void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&hdmi->audio_lock, flags);
+	hdmi->audio_enable = true;
+	hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
+
+void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&hdmi->audio_lock, flags);
+	hdmi->audio_enable = false;
+	hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
+	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_audio_disable);
+
  /*
   * this submodule is responsible for the video data synchronization.
   * for example, for RGB 4:4:4 input, the data map is defined as

@@ -1578,6 +1609,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
  	hdmi->encoder = encoder;
 
  	mutex_init(&hdmi->audio_mutex);
+	spin_lock_init(&hdmi->audio_lock);
 
  	of_property_read_u32(np, "reg-io-width", &val);
 

include/drm/bridge/dw_hdmi.h +0 -0

index 763af51e1d60..bae79f3c4d28 100644

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.

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

At Fri, 22 May 2015 14:15:35 +0100,

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 8 lines

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Sorry, I've been on vacation in the last two weeks, so slowly digesting all backlogs now.

Quoted 3 lines

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

I'm fine to take the sound part. It's only patches 10 and 11, right? Then I can provide a branch that can be merged for the rest drm stuff.

Quoted 33 lines · Takashi Iwai

At Fri, 22 May 2015 14:15:35 +0100,

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 8 lines

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Sorry, I've been on vacation in the last two weeks, so slowly digesting all backlogs now.

Quoted 3 lines

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

Quoted 3 lines

I'm fine to take the sound part. It's only patches 10 and 11, right? Then I can provide a branch that can be merged for the rest drm stuff.

Yep, just patches 10 and 11. If possible, please base these patches on v4.1-rc1, thanks.

At Fri, 22 May 2015 14:53:31 +0100,

Quoted 35 lines · Russell King - ARM Linux

Quoted 33 lines · Takashi Iwai

At Fri, 22 May 2015 14:15:35 +0100,

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 8 lines

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Sorry, I've been on vacation in the last two weeks, so slowly digesting all backlogs now.

Quoted 3 lines

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

Quoted 6 lines

Quoted 3 lines

I'm fine to take the sound part. It's only patches 10 and 11, right? Then I can provide a branch that can be merged for the rest drm stuff.

Yep, just patches 10 and 11. If possible, please base these patches on v4.1-rc1, thanks.

I applied them on top of -rc4, I suppose it's OK?

Quoted 37 lines · Takashi Iwai

At Fri, 22 May 2015 14:53:31 +0100,

Quoted 35 lines · Russell King - ARM Linux

Quoted 33 lines · Takashi Iwai

At Fri, 22 May 2015 14:15:35 +0100,

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 8 lines

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Sorry, I've been on vacation in the last two weeks, so slowly digesting all backlogs now.

Quoted 3 lines

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

I'm fine to take the sound part. It's only patches 10 and 11, right? Then I can provide a branch that can be merged for the rest drm stuff.

Yep, just patches 10 and 11. If possible, please base these patches on v4.1-rc1, thanks.

I applied them on top of -rc4, I suppose it's OK?

It is, but what it means is that I'll keep my copy of the patches in my tree rather than pulling your tree. Having branches spread on different start points makes it difficult to generate a patch series from the git tree - which is something I continue to do for the SolidRun iMX6 platforms (publishing it as separate patches, tarball of those patches and a combined patch.)

As I say, it doesn't matter that much, I can just keep my copies of the patches, and when this stuff gets rebased to 4.2-rc1, git rebase should eliminate them automatically.

At Fri, 22 May 2015 15:00:10 +0100,

Quoted 55 lines · Russell King - ARM Linux

Quoted 37 lines · Takashi Iwai

At Fri, 22 May 2015 14:53:31 +0100,

Quoted 35 lines · Russell King - ARM Linux

Quoted 33 lines · Takashi Iwai

At Fri, 22 May 2015 14:15:35 +0100,

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Mark Brown

Quoted 3 lines · Russell King

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.

As far as I can tell people are fairly happy with the implementation here and unless I'm missing something are definitely happy with the interface. If that's the case can we get this into -next? There's a lot of interest in HDMI right now (which is great) and this would be helpful for that, it seems like even if there are issues with the implementation it would be worth merging as is so we can start adding users and then do any improvements to the interface in parallel.

Quoted 8 lines

Quoted 3 lines

From an interface point of view:

Reviewed-by Mark Brown <[email protected]>

I'd be more than happy if Takashi Iwai wants to take them - I'm not planning on the audio driver itself being merged just yet as we still need to properly hammer out the differences between the AHB audio (for iMX6) and I2S audio (for Rockchip) for this device.

Sorry, I've been on vacation in the last two weeks, so slowly digesting all backlogs now.

Quoted 3 lines

Alternatively, I could move these two patches to the beginning of my series, and merge that point into my for-next and/or publish it as a separate sub-branch... whatever people want, just let me know.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

I'm fine to take the sound part. It's only patches 10 and 11, right? Then I can provide a branch that can be merged for the rest drm stuff.

Yep, just patches 10 and 11. If possible, please base these patches on v4.1-rc1, thanks.

I applied them on top of -rc4, I suppose it's OK?

It is, but what it means is that I'll keep my copy of the patches in my tree rather than pulling your tree. Having branches spread on different start points makes it difficult to generate a patch series from the git tree - which is something I continue to do for the SolidRun iMX6 platforms (publishing it as separate patches, tarball of those patches and a combined patch.)

Quoted 3 lines

As I say, it doesn't matter that much, I can just keep my copies of the patches, and when this stuff gets rebased to 4.2-rc1, git rebase should eliminate them automatically.

OK, if so, then I rebase on top of -rc1. The branch isn't merged yet, so no big problem.

At Fri, 22 May 2015 16:02:13 +0200,

Quoted 2 lines · Takashi Iwai

OK, if so, then I rebase on top of -rc1. The branch isn't merged yet, so no big problem.

Now the rebased branch is pushed.

I also merged this branch now to for-next branch, so that it'll be tested through linux-next.

Add a helper to create the IEC958 channel status from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.

Add ALSA based HDMI AHB audio driver for dw_hdmi. The only buffer format supported by the hardware is its own special IEC958 based format, which is not compatible with any ALSA format. To avoid doing too much data manipulation within the driver, we support only ALSAs IEC958 LE and 24-bit PCM formats for 2 to 6 channels, which we convert to its hardware format.

A more desirable solution would be to have this conversion in userspace, but ALSA does not appear to allow such transformations outside of libasound itself.

Signed-off-by Russell King <[email protected]>

---
 drivers/gpu/drm/bridge/Kconfig             |  10 +
 drivers/gpu/drm/bridge/Makefile            |   1 +
 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c | 560 +++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h |  13 +
 drivers/gpu/drm/bridge/dw_hdmi.c           |  24 ++
 drivers/gpu/drm/bridge/dw_hdmi.h           |   3 +
 6 files changed, 611 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c
 create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h

drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c new file +6 -0

+static struct snd_pcm_hardware dw_hdmi_hw = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID,
+	.formats = SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |

(Of course having userspace set them requires that the device has a proper entry in /usr/share/alsa/cards and the pcm device is accessed via the standard "hdmi" or "iec958" device names which perform the channel status word setup. I guess the ARM SoC stuff generally doesn't bother with that, explaining a bit why some kernel drivers set them by themselves).

HDMI_FC_AUDICONF2 register default value is 0x00, which means plain stereo (per CEA-861). If this is what goes on to the HDMI link as well, the audio sink should ignore the other channels.
Did you check that multichannel PCM actually works? (maybe I'm just missing where CA is set)

Quoted 5 lines

(Of course having userspace set them requires that the device has a proper entry in /usr/share/alsa/cards and the pcm device is accessed via the standard "hdmi" or "iec958" device names which perform the channel status word setup. I guess the ARM SoC stuff generally doesn't bother with that, explaining a bit why some kernel drivers set them by themselves).

I'm not sure that's sufficient - I haven't yet found where in the ALSA userspace, the AES bits are appropriately set according to the sample rate.

09.05.2015, 19:55, Russell King - ARM Linux kirjoitti:

Quoted 7 lines · Anssi Hannula

Quoted 5 lines

(Of course having userspace set them requires that the device has a proper entry in /usr/share/alsa/cards and the pcm device is accessed via the standard "hdmi" or "iec958" device names which perform the channel status word setup. I guess the ARM SoC stuff generally doesn't bother with that, explaining a bit why some kernel drivers set them by themselves).

Quoted 3 lines

I'm not sure that's sufficient - I haven't yet found where in the ALSA userspace, the AES bits are appropriately set according to the sample rate.

Right, that is left to the applications (e.g. VLC and Kodi do that). I'm under the impression that sinks do not normally care about this value, though, but that could just be because most desktop HW sets it by themselves.

Even VLC _doesn't_ if it's outputting to a standard audio - in other words, if you don't tick the SPDIF direct output option which defaults to disabled (which, when enabled, opens the device passing the AES bits _and_ permits it to send a compressed audio stream.) I've looked at this in VLC many times...

Quoted 8 lines

09.05.2015, 19:55, Russell King - ARM Linux kirjoitti:

Quoted 7 lines · Anssi Hannula

Quoted 5 lines

(Of course having userspace set them requires that the device has a proper entry in /usr/share/alsa/cards and the pcm device is accessed via the standard "hdmi" or "iec958" device names which perform the channel status word setup. I guess the ARM SoC stuff generally doesn't bother with that, explaining a bit why some kernel drivers set them by themselves).

Quoted 8 lines

Quoted 3 lines

I'm not sure that's sufficient - I haven't yet found where in the ALSA userspace, the AES bits are appropriately set according to the sample rate.

Right, that is left to the applications (e.g. VLC and Kodi do that). I'm under the impression that sinks do not normally care about this value, though, but that could just be because most desktop HW sets it by themselves.

No, that seems totally wrong to me.

What if you open the device using aplay? Or pulseaudio? Or madplay? Or another audio application which thinks it's addressing a standard PCM device.

Why should every audio user have some code in it to generate the IEC bits?

Even VLC _doesn't_ if it's outputting to a standard audio - in other words, if you don't tick the SPDIF direct output option which defaults to disabled (which, when enabled, opens the device passing the AES bits _and_ permits it to send a compressed audio stream.) I've looked at this in VLC many times...

I think you're a little confused about what userspace does here.