[PATCH RFC 0/4] HDA/i915 jack handling using component

[PATCH RFC 0/4] HDA/i915 jack handling using component

[Takashi Iwai]

Hi,

after the previous discussion ([RFC PATCH 0/2] ALSA: hda - DP MST
audio for Jack support), I thought it'd be even easier to give a
concrete code for further discussion; so here we go, it's a patchset
to implement the jack via audio component.

In the end, I added a new audio component ops to fetch the current
ELD, since the state is unknown at the time the audio driver starts.
The other two patches are just a cleanup.

This series is submitted just as an RFC, basically for a better
understanding, not seriously considered to be merged as is.
(That's why no intel-gfx is Cc'ed yet.)

Note that the i915 patches are applied to the latest Linus tree, while
patch_hdmi.c patch is to my for-linus branch.  The patches are found
in test/hdmi-jack branch of sound git tree.


Takashi

===

Takashi Iwai (4):
  drm/i915: Add get_eld audio component
  drm/i915: Remove superfluous NULL check
  drm/i915: refactoring audio component functions
  ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling

 drivers/gpu/drm/i915/intel_audio.c | 73 ++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 include/drm/i915_component.h       |  4 ++
 sound/pci/hda/patch_hdmi.c         | 83 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 141 insertions(+), 20 deletions(-)

-- 
2.6.3

[PATCH RFC 1/4] drm/i915: Add get_eld audio component

[Takashi Iwai]

Implement a new i915_audio_component_ops, get_eld().  It's called by
the audio driver to fetch the current ELD of the given HDMI/DP port.
The return value zero indicates that the audio is off, otherwise a
positive value as the ELD byte size or a negative error code.

For achieving this, a new field audio_enabled is added to struct
intel_digital_port.  This is set/reset at each audio enable/disable
call in intel_audio.c.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_audio.c | 39 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 include/drm/i915_component.h       |  4 ++++
 3 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 4dccd9b003a1..ec81f8481d25 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 
 	connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
 
+	intel_dig_port->audio_enabled = true;
 	if (dev_priv->display.audio_codec_enable)
 		dev_priv->display.audio_codec_enable(connector, intel_encoder,
 						     adjusted_mode);
@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	enum port port = intel_dig_port->port;
 
+	intel_dig_port->audio_enabled = false;
 	if (dev_priv->display.audio_codec_disable)
 		dev_priv->display.audio_codec_disable(intel_encoder);
 
@@ -706,6 +708,42 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 	return 0;
 }
 
+static int i915_audio_component_get_eld(struct device *dev, int port,
+					unsigned char *buf, int max_bytes)
+{
+	struct drm_i915_private *dev_priv = dev_to_i915(dev);
+	struct drm_device *drm_dev = dev_priv->dev;
+	struct intel_encoder *intel_encoder;
+	struct intel_digital_port *intel_dig_port;
+	struct drm_connector *connector;
+	unsigned char *eld;
+	int ret = -EINVAL;
+
+	mutex_lock(&dev_priv->av_mutex);
+	for_each_intel_encoder(drm_dev, intel_encoder) {
+		if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
+		    intel_encoder->type != INTEL_OUTPUT_HDMI)
+			continue;
+		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+		if (port == intel_dig_port->port) {
+			if (!intel_dig_port->audio_enabled) {
+				ret = 0;
+				break;
+			}
+			connector = drm_select_eld(&intel_encoder->base);
+			if (!connector)
+				break;
+			eld = connector->eld;
+			ret = min(max_bytes, drm_eld_size(eld));
+			memcpy(buf, eld, ret);
+			break;
+		}
+	}
+
+	mutex_unlock(&dev_priv->av_mutex);
+	return ret;
+}
+
 static const struct i915_audio_component_ops i915_audio_component_ops = {
 	.owner		= THIS_MODULE,
 	.get_power	= i915_audio_component_get_power,
@@ -713,6 +751,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = {
 	.codec_wake_override = i915_audio_component_codec_wake_override,
 	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
 	.sync_audio_rate = i915_audio_component_sync_audio_rate,
+	.get_eld	= i915_audio_component_get_eld,
 };
 
 static int i915_audio_component_bind(struct device *i915_dev,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0598932ce623..4afc7560be04 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -798,6 +798,7 @@ struct intel_digital_port {
 	u32 saved_port_bits;
 	struct intel_dp dp;
 	struct intel_hdmi hdmi;
+	bool audio_enabled;
 	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
 	bool release_cl2_override;
 };
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index 30d89e0da2c6..0165eab87dbe 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -38,6 +38,8 @@
  * @codec_wake_override: Enable/Disable generating the codec wake signal
  * @get_cdclk_freq: get the Core Display Clock in KHz
  * @sync_audio_rate: set n/cts based on the sample rate
+ * @get_eld: fill ELD bytes for the given port, return the size of ELD bytes,
+ *           zero if unconnected, or a negative error code.
  */
 struct i915_audio_component_ops {
 	struct module *owner;
@@ -46,6 +48,8 @@ struct i915_audio_component_ops {
 	void (*codec_wake_override)(struct device *, bool enable);
 	int (*get_cdclk_freq)(struct device *);
 	int (*sync_audio_rate)(struct device *, int port, int rate);
+	int (*get_eld)(struct device *, int port, unsigned char *buf,
+		       int max_bytes);
 };
 
 struct i915_audio_component_audio_ops {
-- 
2.6.3

[PATCH RFC 2/4] drm/i915: Remove superfluous NULL check

[Takashi Iwai]

to_intel_crtc() always returns a non-NULL pointer.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_audio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index ec81f8481d25..dc8141324fcd 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -658,10 +658,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
 		if (port == intel_dig_port->port) {
 			crtc = to_intel_crtc(intel_encoder->base.crtc);
-			if (!crtc) {
-				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
-				continue;
-			}
 			pipe = crtc->pipe;
 			break;
 		}
-- 
2.6.3

[PATCH RFC 3/4] drm/i915: refactoring audio component functions

[Takashi Iwai]

We have a common loop of encoder to look for the given audio port in
two audio component functions.  Split out a local helper function to
do it for the code simplification.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_audio.c | 60 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index dc8141324fcd..8169693418af 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -630,17 +630,33 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
 	return ret;
 }
 
+static struct intel_encoder *audio_port_to_encoder(struct drm_device *drm_dev,
+						   int port)
+{
+	struct intel_encoder *intel_encoder;
+	struct intel_digital_port *intel_dig_port;
+
+	for_each_intel_encoder(drm_dev, intel_encoder) {
+		if (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+		    intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
+			continue;
+		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+		if (port == intel_dig_port->port)
+			return intel_encoder;
+	}
+	return NULL;
+}
+
 static int i915_audio_component_sync_audio_rate(struct device *dev,
 						int port, int rate)
 {
 	struct drm_i915_private *dev_priv = dev_to_i915(dev);
 	struct drm_device *drm_dev = dev_priv->dev;
 	struct intel_encoder *intel_encoder;
-	struct intel_digital_port *intel_dig_port;
 	struct intel_crtc *crtc;
 	struct drm_display_mode *mode;
 	struct i915_audio_component *acomp = dev_priv->audio_component;
-	enum pipe pipe = -1;
+	enum pipe pipe;
 	u32 tmp;
 	int n;
 
@@ -652,22 +668,14 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 
 	mutex_lock(&dev_priv->av_mutex);
 	/* 1. get the pipe */
-	for_each_intel_encoder(drm_dev, intel_encoder) {
-		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
-			continue;
-		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
-		if (port == intel_dig_port->port) {
-			crtc = to_intel_crtc(intel_encoder->base.crtc);
-			pipe = crtc->pipe;
-			break;
-		}
-	}
-
-	if (pipe == INVALID_PIPE) {
+	intel_encoder = audio_port_to_encoder(drm_dev, port);
+	if (!intel_encoder || intel_encoder->type != INTEL_OUTPUT_HDMI) {
 		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
 		mutex_unlock(&dev_priv->av_mutex);
 		return -ENODEV;
 	}
+	crtc = to_intel_crtc(intel_encoder->base.crtc);
+	pipe = crtc->pipe;
 	DRM_DEBUG_KMS("pipe %c connects port %c\n",
 				  pipe_name(pipe), port_name(port));
 	mode = &crtc->config->base.adjusted_mode;
@@ -716,23 +724,17 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 	int ret = -EINVAL;
 
 	mutex_lock(&dev_priv->av_mutex);
-	for_each_intel_encoder(drm_dev, intel_encoder) {
-		if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
-		    intel_encoder->type != INTEL_OUTPUT_HDMI)
-			continue;
+	intel_encoder = audio_port_to_encoder(drm_dev, port);
+	if (intel_encoder) {
+		ret = 0;
 		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
-		if (port == intel_dig_port->port) {
-			if (!intel_dig_port->audio_enabled) {
-				ret = 0;
-				break;
-			}
+		if (intel_dig_port->audio_enabled) {
 			connector = drm_select_eld(&intel_encoder->base);
-			if (!connector)
-				break;
-			eld = connector->eld;
-			ret = min(max_bytes, drm_eld_size(eld));
-			memcpy(buf, eld, ret);
-			break;
+			if (connector) {
+				eld = connector->eld;
+				ret = min(max_bytes, drm_eld_size(eld));
+				memcpy(buf, eld, ret);
+			}
 		}
 	}
 
-- 
2.6.3

[PATCH RFC 4/4] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling

[Takashi Iwai]

Since we have a new audio component ops to fetch the current ELD and
state now, we can reduce the usage of unsol event of HDMI/DP pins.
The unsol event isn't only unreliable, but it also needs the power
up/down of the codec and link at each time, which is a significant
power and time loss.

In this patch, the jack creation and unsol/jack event handling are
modified to use the audio component for the dedicated Intel chips.

The jack handling got slightly more codes than a simple usage of
hda_jack layer since we need to deal directly with snd_jack object;
the hda_jack layer is basically designed for the pin sense read and
unsol events, both of which aren't used any longer in our case.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 60cd9e700909..ca1d2d4a295e 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct snd_jack *acomp_jack; /* jack via audio component */
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -141,6 +142,7 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
+	bool use_acomp; /* use audio component for ELD notify/update */
 
 	/*
 	 * Non-generic VIA/NVIDIA specific
@@ -1530,6 +1532,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 	return 0;
 }
 
+static void sync_eld_via_acomp(struct hda_codec *codec,
+			       struct hdmi_spec_per_pin *per_pin);
+
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 {
 	struct hda_jack_tbl *jack;
@@ -1551,6 +1556,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	bool eld_changed = false;
 	bool ret;
 
+	if (spec->use_acomp) {
+		sync_eld_via_acomp(codec, per_pin);
+		return false; /* don't call snd_hda_jack_report_sync() */
+	}
+
 	snd_hda_power_up_pm(codec);
 	present = snd_hda_pin_sense(codec, pin_nid);
 
@@ -2091,6 +2101,67 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec)
 	return 0;
 }
 
+/* update ELD and jack state via audio component */
+static void sync_eld_via_acomp(struct hda_codec *codec,
+			       struct hdmi_spec_per_pin *per_pin)
+{
+	struct i915_audio_component *acomp = codec->bus->core.audio_component;
+	struct hdmi_eld *eld = &per_pin->sink_eld;
+	int size;
+
+	if (acomp && acomp->ops && acomp->ops->get_eld) {
+		size = acomp->ops->get_eld(acomp->dev,
+					   intel_pin2port(per_pin->pin_nid),
+					   eld->eld_buffer,
+					   ELD_MAX_SIZE);
+		if (size < 0)
+			return;
+		if (size) {
+			memset(&eld->info, 0, sizeof(eld->info));
+			if (snd_hdmi_parse_eld(codec, &eld->info,
+					       eld->eld_buffer,
+					       size) < 0)
+				size = 0;
+		}
+		if (size) {
+			eld->monitor_present = true;
+			eld->eld_valid = true;
+			eld->eld_size = size;
+
+		} else {
+			eld->monitor_present = false;
+			eld->eld_valid = false;
+			eld->eld_size = 0;
+		}
+		snd_jack_report(per_pin->acomp_jack,
+				size ? SND_JACK_AVOUT : 0);
+	}
+}
+
+static void free_acomp_jack_priv(struct snd_jack *jack)
+{
+	struct hdmi_spec_per_pin *per_pin = jack->private_data;
+
+	per_pin->acomp_jack = NULL;
+}
+
+static int add_acomp_jack_kctl(struct hda_codec *codec,
+			       struct hdmi_spec_per_pin *per_pin,
+			       const char *name)
+{
+	struct snd_jack *jack;
+	int err;
+
+	err = snd_jack_new(codec->card, name, SND_JACK_AVOUT, &jack,
+			   true, false);
+	if (err < 0)
+		return err;
+	per_pin->acomp_jack = jack;
+	jack->private_data = per_pin;
+	jack->private_free = free_acomp_jack_priv;
+	return 0;
+}
+
 static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
 {
 	char hdmi_str[32] = "HDMI/DP";
@@ -2101,6 +2172,8 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
 
 	if (pcmdev > 0)
 		sprintf(hdmi_str + strlen(hdmi_str), ",pcm=%d", pcmdev);
+	if (spec->use_acomp)
+		return add_acomp_jack_kctl(codec, per_pin, hdmi_str);
 	phantom_jack = !is_jack_detectable(codec, per_pin->pin_nid);
 	if (phantom_jack)
 		strncat(hdmi_str, " Phantom",
@@ -2196,6 +2269,8 @@ static int generic_hdmi_init(struct hda_codec *codec)
 		hda_nid_t pin_nid = per_pin->pin_nid;
 
 		hdmi_init_pin(codec, pin_nid);
+		if (spec->use_acomp)
+			continue;
 		snd_hda_jack_detect_enable_callback(codec, pin_nid,
 			codec->jackpoll_interval > 0 ? jack_callback : NULL);
 	}
@@ -2219,7 +2294,7 @@ static void generic_hdmi_free(struct hda_codec *codec)
 	struct hdmi_spec *spec = codec->spec;
 	int pin_idx;
 
-	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+	if (spec->use_acomp)
 		snd_hdac_i915_register_notifier(NULL);
 
 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
@@ -2227,6 +2302,8 @@ static void generic_hdmi_free(struct hda_codec *codec)
 
 		cancel_delayed_work_sync(&per_pin->work);
 		eld_proc_free(per_pin);
+		if (per_pin->acomp_jack)
+			snd_device_free(codec->card, per_pin->acomp_jack);
 	}
 
 	hdmi_array_free(spec);
@@ -2381,7 +2458,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 	if (is_valleyview_plus(codec) || is_skylake(codec))
 		codec->core.link_power_control = 1;
 
-	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
+	spec->use_acomp =
+		is_haswell_plus(codec) || is_valleyview_plus(codec);
+	if (spec->use_acomp) {
 		codec->depop_delay = 0;
 		spec->i915_audio_ops.audio_ptr = codec;
 		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
-- 
2.6.3

Re: [PATCH RFC 0/4] HDA/i915 jack handling using component

[David Henningsson]

On 2015-11-12 17:20, Takashi Iwai wrote:
> Hi,
>
> after the previous discussion ([RFC PATCH 0/2] ALSA: hda - DP MST
> audio for Jack support), I thought it'd be even easier to give a
> concrete code for further discussion; so here we go, it's a patchset
> to implement the jack via audio component.
>
> In the end, I added a new audio component ops to fetch the current
> ELD, since the state is unknown at the time the audio driver starts.
> The other two patches are just a cleanup.
>
> This series is submitted just as an RFC, basically for a better
> understanding, not seriously considered to be merged as is.
> (That's why no intel-gfx is Cc'ed yet.)
>
> Note that the i915 patches are applied to the latest Linus tree, while
> patch_hdmi.c patch is to my for-linus branch.  The patches are found
> in test/hdmi-jack branch of sound git tree.

Right; maybe you got tired of waiting for me to make that patch set, 
sorry. :-/

I looked at it a while ago, but got a bit stuck in figuring out how to 
properly lock things to ensure that the ELD info is not being written to 
(by another kernel thread) at the same time as we read it.

I'm not sure whether your patch resolves that or not. But if av_mutex is 
always held whenever your audio_enabled flag is changed (and ELD is 
never changed while audio_enabled=true), that might do it though.

We might also need to send audio_enabled itself over too, not sure if we 
can have audio enabled without valid ELD - in some cases when we have 
monitors not giving us any ELD info.

>
>
> Takashi
>
> ===
>
> Takashi Iwai (4):
>    drm/i915: Add get_eld audio component
>    drm/i915: Remove superfluous NULL check
>    drm/i915: refactoring audio component functions
>    ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling
>
>   drivers/gpu/drm/i915/intel_audio.c | 73 ++++++++++++++++++++++++---------
>   drivers/gpu/drm/i915/intel_drv.h   |  1 +
>   include/drm/i915_component.h       |  4 ++
>   sound/pci/hda/patch_hdmi.c         | 83 +++++++++++++++++++++++++++++++++++++-
>   4 files changed, 141 insertions(+), 20 deletions(-)
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH RFC 0/4] HDA/i915 jack handling using component

[Takashi Iwai]

On Thu, 12 Nov 2015 17:44:04 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-11-12 17:20, Takashi Iwai wrote:
> > Hi,
> >
> > after the previous discussion ([RFC PATCH 0/2] ALSA: hda - DP MST
> > audio for Jack support), I thought it'd be even easier to give a
> > concrete code for further discussion; so here we go, it's a patchset
> > to implement the jack via audio component.
> >
> > In the end, I added a new audio component ops to fetch the current
> > ELD, since the state is unknown at the time the audio driver starts.
> > The other two patches are just a cleanup.
> >
> > This series is submitted just as an RFC, basically for a better
> > understanding, not seriously considered to be merged as is.
> > (That's why no intel-gfx is Cc'ed yet.)
> >
> > Note that the i915 patches are applied to the latest Linus tree, while
> > patch_hdmi.c patch is to my for-linus branch.  The patches are found
> > in test/hdmi-jack branch of sound git tree.
> 
> Right; maybe you got tired of waiting for me to make that patch set, 
> sorry. :-/

Don't worry, I was just bored at an afternoon coffee time, so started
hacking quickly as a proof of concept.

> I looked at it a while ago, but got a bit stuck in figuring out how to 
> properly lock things to ensure that the ELD info is not being written to 
> (by another kernel thread) at the same time as we read it.

Yeah, the locking needs to be revised later in my patchset, too.

> I'm not sure whether your patch resolves that or not. But if av_mutex is 
> always held whenever your audio_enabled flag is changed (and ELD is 
> never changed while audio_enabled=true), that might do it though.
> 
> We might also need to send audio_enabled itself over too, not sure if we 
> can have audio enabled without valid ELD - in some cases when we have 
> monitors not giving us any ELD info.

It's a good question.  We currently allow playback even on a pin that
didn't get ELD, indeed.  So, yes, it might be worth to return both
values.


thanks,

Takashi

Re: [PATCH RFC 4/4] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling

[Libin Yang]

On 11/13/2015 12:20 AM, Takashi Iwai wrote:
> Since we have a new audio component ops to fetch the current ELD and
> state now, we can reduce the usage of unsol event of HDMI/DP pins.
> The unsol event isn't only unreliable, but it also needs the power
> up/down of the codec and link at each time, which is a significant
> power and time loss.
>
> In this patch, the jack creation and unsol/jack event handling are
> modified to use the audio component for the dedicated Intel chips.
>
> The jack handling got slightly more codes than a simple usage of
> hda_jack layer since we need to deal directly with snd_jack object;
> the hda_jack layer is basically designed for the pin sense read and
> unsol events, both of which aren't used any longer in our case.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/pci/hda/patch_hdmi.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 60cd9e700909..ca1d2d4a295e 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
>   	struct mutex lock;
>   	struct delayed_work work;
>   	struct snd_kcontrol *eld_ctl;
> +	struct snd_jack *acomp_jack; /* jack via audio component */
>   	int repoll_count;
>   	bool setup; /* the stream has been set up by prepare callback */
>   	int channels; /* current number of channels */
> @@ -141,6 +142,7 @@ struct hdmi_spec {
>   	struct hdmi_ops ops;
>
>   	bool dyn_pin_out;
> +	bool use_acomp; /* use audio component for ELD notify/update */
>
>   	/*
>   	 * Non-generic VIA/NVIDIA specific
> @@ -1530,6 +1532,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
>   	return 0;
>   }
>
> +static void sync_eld_via_acomp(struct hda_codec *codec,
> +			       struct hdmi_spec_per_pin *per_pin);
> +
>   static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>   {
>   	struct hda_jack_tbl *jack;
> @@ -1551,6 +1556,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>   	bool eld_changed = false;
>   	bool ret;
>
> +	if (spec->use_acomp) {
> +		sync_eld_via_acomp(codec, per_pin);
> +		return false; /* don't call snd_hda_jack_report_sync() */

I think we still need call intel_verify_pin_cvt_connect(), 
intel_not_share_assigned_cvt() and hdmi_setup_audio_infoframe() as 
before if eld_valid is true.


> +	}
> +
>   	snd_hda_power_up_pm(codec);
>   	present = snd_hda_pin_sense(codec, pin_nid);
>
> @@ -2091,6 +2101,67 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec)
>   	return 0;
>   }
>
> +/* update ELD and jack state via audio component */
> +static void sync_eld_via_acomp(struct hda_codec *codec,
> +			       struct hdmi_spec_per_pin *per_pin)
> +{
> +	struct i915_audio_component *acomp = codec->bus->core.audio_component;
> +	struct hdmi_eld *eld = &per_pin->sink_eld;
> +	int size;
> +
> +	if (acomp && acomp->ops && acomp->ops->get_eld) {
> +		size = acomp->ops->get_eld(acomp->dev,
> +					   intel_pin2port(per_pin->pin_nid),
> +					   eld->eld_buffer,
> +					   ELD_MAX_SIZE);
> +		if (size < 0)
> +			return;
> +		if (size) {
> +			memset(&eld->info, 0, sizeof(eld->info));
> +			if (snd_hdmi_parse_eld(codec, &eld->info,
> +					       eld->eld_buffer,
> +					       size) < 0)
> +				size = 0;
> +		}
> +		if (size) {
> +			eld->monitor_present = true;
> +			eld->eld_valid = true;
> +			eld->eld_size = size;
> +
> +		} else {
> +			eld->monitor_present = false;
> +			eld->eld_valid = false;
> +			eld->eld_size = 0;
> +		}
> +		snd_jack_report(per_pin->acomp_jack,
> +				size ? SND_JACK_AVOUT : 0);


Is Jack report enough? Do we need call snd_ctl_notify() to notify eld 
change event as before?

Best Regards,
Libin

> +	}
> +}
> +
> +static void free_acomp_jack_priv(struct snd_jack *jack)
> +{
> +	struct hdmi_spec_per_pin *per_pin = jack->private_data;
> +
> +	per_pin->acomp_jack = NULL;
> +}
> +
> +static int add_acomp_jack_kctl(struct hda_codec *codec,
> +			       struct hdmi_spec_per_pin *per_pin,
> +			       const char *name)
> +{
> +	struct snd_jack *jack;
> +	int err;
> +
> +	err = snd_jack_new(codec->card, name, SND_JACK_AVOUT, &jack,
> +			   true, false);
> +	if (err < 0)
> +		return err;
> +	per_pin->acomp_jack = jack;
> +	jack->private_data = per_pin;
> +	jack->private_free = free_acomp_jack_priv;
> +	return 0;
> +}
> +
>   static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
>   {
>   	char hdmi_str[32] = "HDMI/DP";
> @@ -2101,6 +2172,8 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
>
>   	if (pcmdev > 0)
>   		sprintf(hdmi_str + strlen(hdmi_str), ",pcm=%d", pcmdev);
> +	if (spec->use_acomp)
> +		return add_acomp_jack_kctl(codec, per_pin, hdmi_str);
>   	phantom_jack = !is_jack_detectable(codec, per_pin->pin_nid);
>   	if (phantom_jack)
>   		strncat(hdmi_str, " Phantom",
> @@ -2196,6 +2269,8 @@ static int generic_hdmi_init(struct hda_codec *codec)
>   		hda_nid_t pin_nid = per_pin->pin_nid;
>
>   		hdmi_init_pin(codec, pin_nid);
> +		if (spec->use_acomp)
> +			continue;
>   		snd_hda_jack_detect_enable_callback(codec, pin_nid,
>   			codec->jackpoll_interval > 0 ? jack_callback : NULL);
>   	}
> @@ -2219,7 +2294,7 @@ static void generic_hdmi_free(struct hda_codec *codec)
>   	struct hdmi_spec *spec = codec->spec;
>   	int pin_idx;
>
> -	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> +	if (spec->use_acomp)
>   		snd_hdac_i915_register_notifier(NULL);
>
>   	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> @@ -2227,6 +2302,8 @@ static void generic_hdmi_free(struct hda_codec *codec)
>
>   		cancel_delayed_work_sync(&per_pin->work);
>   		eld_proc_free(per_pin);
> +		if (per_pin->acomp_jack)
> +			snd_device_free(codec->card, per_pin->acomp_jack);
>   	}
>
>   	hdmi_array_free(spec);
> @@ -2381,7 +2458,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
>   	if (is_valleyview_plus(codec) || is_skylake(codec))
>   		codec->core.link_power_control = 1;
>
> -	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> +	spec->use_acomp =
> +		is_haswell_plus(codec) || is_valleyview_plus(codec);
> +	if (spec->use_acomp) {
>   		codec->depop_delay = 0;
>   		spec->i915_audio_ops.audio_ptr = codec;
>   		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
>

Re: [PATCH RFC 4/4] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling

[Takashi Iwai]

On Fri, 13 Nov 2015 02:55:25 +0100,
Libin Yang wrote:
> 
> 
> On 11/13/2015 12:20 AM, Takashi Iwai wrote:
> > Since we have a new audio component ops to fetch the current ELD and
> > state now, we can reduce the usage of unsol event of HDMI/DP pins.
> > The unsol event isn't only unreliable, but it also needs the power
> > up/down of the codec and link at each time, which is a significant
> > power and time loss.
> >
> > In this patch, the jack creation and unsol/jack event handling are
> > modified to use the audio component for the dedicated Intel chips.
> >
> > The jack handling got slightly more codes than a simple usage of
> > hda_jack layer since we need to deal directly with snd_jack object;
> > the hda_jack layer is basically designed for the pin sense read and
> > unsol events, both of which aren't used any longer in our case.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   sound/pci/hda/patch_hdmi.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 81 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 60cd9e700909..ca1d2d4a295e 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
> >   	struct mutex lock;
> >   	struct delayed_work work;
> >   	struct snd_kcontrol *eld_ctl;
> > +	struct snd_jack *acomp_jack; /* jack via audio component */
> >   	int repoll_count;
> >   	bool setup; /* the stream has been set up by prepare callback */
> >   	int channels; /* current number of channels */
> > @@ -141,6 +142,7 @@ struct hdmi_spec {
> >   	struct hdmi_ops ops;
> >
> >   	bool dyn_pin_out;
> > +	bool use_acomp; /* use audio component for ELD notify/update */
> >
> >   	/*
> >   	 * Non-generic VIA/NVIDIA specific
> > @@ -1530,6 +1532,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
> >   	return 0;
> >   }
> >
> > +static void sync_eld_via_acomp(struct hda_codec *codec,
> > +			       struct hdmi_spec_per_pin *per_pin);
> > +
> >   static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >   {
> >   	struct hda_jack_tbl *jack;
> > @@ -1551,6 +1556,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >   	bool eld_changed = false;
> >   	bool ret;
> >
> > +	if (spec->use_acomp) {
> > +		sync_eld_via_acomp(codec, per_pin);
> > +		return false; /* don't call snd_hda_jack_report_sync() */
> 
> I think we still need call intel_verify_pin_cvt_connect(), 
> intel_not_share_assigned_cvt() and hdmi_setup_audio_infoframe() as 
> before if eld_valid is true.

Right, also ELD kctl notification is missing, too.
It implies that we'd better refactor the ELD update code, split the
code from hdmi_present_sense() to do both commonly.


Takashi

Re: [PATCH RFC 4/4] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling

[Takashi Iwai]

On Fri, 13 Nov 2015 06:56:17 +0100,
Takashi Iwai wrote:
> 
> On Fri, 13 Nov 2015 02:55:25 +0100,
> Libin Yang wrote:
> > 
> > 
> > On 11/13/2015 12:20 AM, Takashi Iwai wrote:
> > > Since we have a new audio component ops to fetch the current ELD and
> > > state now, we can reduce the usage of unsol event of HDMI/DP pins.
> > > The unsol event isn't only unreliable, but it also needs the power
> > > up/down of the codec and link at each time, which is a significant
> > > power and time loss.
> > >
> > > In this patch, the jack creation and unsol/jack event handling are
> > > modified to use the audio component for the dedicated Intel chips.
> > >
> > > The jack handling got slightly more codes than a simple usage of
> > > hda_jack layer since we need to deal directly with snd_jack object;
> > > the hda_jack layer is basically designed for the pin sense read and
> > > unsol events, both of which aren't used any longer in our case.
> > >
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >   sound/pci/hda/patch_hdmi.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 81 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index 60cd9e700909..ca1d2d4a295e 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
> > >   	struct mutex lock;
> > >   	struct delayed_work work;
> > >   	struct snd_kcontrol *eld_ctl;
> > > +	struct snd_jack *acomp_jack; /* jack via audio component */
> > >   	int repoll_count;
> > >   	bool setup; /* the stream has been set up by prepare callback */
> > >   	int channels; /* current number of channels */
> > > @@ -141,6 +142,7 @@ struct hdmi_spec {
> > >   	struct hdmi_ops ops;
> > >
> > >   	bool dyn_pin_out;
> > > +	bool use_acomp; /* use audio component for ELD notify/update */
> > >
> > >   	/*
> > >   	 * Non-generic VIA/NVIDIA specific
> > > @@ -1530,6 +1532,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
> > >   	return 0;
> > >   }
> > >
> > > +static void sync_eld_via_acomp(struct hda_codec *codec,
> > > +			       struct hdmi_spec_per_pin *per_pin);
> > > +
> > >   static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> > >   {
> > >   	struct hda_jack_tbl *jack;
> > > @@ -1551,6 +1556,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> > >   	bool eld_changed = false;
> > >   	bool ret;
> > >
> > > +	if (spec->use_acomp) {
> > > +		sync_eld_via_acomp(codec, per_pin);
> > > +		return false; /* don't call snd_hda_jack_report_sync() */
> > 
> > I think we still need call intel_verify_pin_cvt_connect(), 
> > intel_not_share_assigned_cvt() and hdmi_setup_audio_infoframe() as 
> > before if eld_valid is true.
> 
> Right, also ELD kctl notification is missing, too.
> It implies that we'd better refactor the ELD update code, split the
> code from hdmi_present_sense() to do both commonly.

FYI, I updated the patchset on test/hdmi-jack branch now.
It receives audio_enabled state, and handles ELD update more
properly.


Takashi