[PATCH 0/7] Add get_eld audio component for i915/HD-audio

[PATCH 0/7] Add get_eld audio component for i915/HD-audio

[Takashi Iwai]

Hi,

this is a patchset to add get_eld op to audio component for
communicating more directly between i915 and HD-audio.  Currently, the
HDMI/DP audio status and ELD are notified and obtained via the
hardware-level communication over HD-audio unsolicited event and
verbs although the graphics driver holds the exactly same
information.  As we already have a notification via audio component,
this is another step forward; the audio driver fetches directly the
audio status and ELD via the new component op.

The initial draft was posted to alsa-devel as an RFC some weeks ago,
and this is a revised version, posted to both i915 and alsa-devel for
review.

The current patchset is found in sound git tree test/hdmi-jack
branch.


Takashi

Takashi Iwai (7):
  drm/i915: Remove superfluous NULL check
  drm/i915: Add get_eld audio component
  drm/i915: refactoring audio component functions
  ALSA: hda - Split ELD update code from hdmi_present_sense()
  ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling
  ALSA: hda - Do zero-clear in snd_hdmi_parse_eld() itself
  ALSA: hda - Skip ELD notification during PM process

 drivers/gpu/drm/i915/intel_audio.c |  75 ++++++++++----
 drivers/gpu/drm/i915/intel_drv.h   |   1 +
 include/drm/i915_component.h       |   3 +
 sound/pci/hda/hda_eld.c            |   1 +
 sound/pci/hda/patch_hdmi.c         | 195 ++++++++++++++++++++++++++-----------
 5 files changed, 200 insertions(+), 75 deletions(-)

-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] 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 4dccd9b003a1..0c38cc6c82ae 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -656,10 +656,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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] 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.
It returns the size of ELD bytes if it's valid, or zero if the audio
is disabled or unplugged, 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 | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 include/drm/i915_component.h       |  3 +++
 3 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 0c38cc6c82ae..6b318a8d5dc9 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);
 
@@ -702,6 +704,43 @@ 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,
+					bool *enabled,
+					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) {
+			ret = 0;
+			*enabled = intel_dig_port->audio_enabled;
+			if (!*enabled)
+				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,
@@ -709,6 +748,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..058d39e8d57f 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -38,6 +38,7 @@
  * @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 the audio state and ELD bytes for the given port
  */
 struct i915_audio_component_ops {
 	struct module *owner;
@@ -46,6 +47,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, bool *enabled,
+		       unsigned char *buf, int max_bytes);
 };
 
 struct i915_audio_component_audio_ops {
-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] 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 | 61 ++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 6b318a8d5dc9..024e88ae6305 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;
@@ -717,23 +725,18 @@ 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) {
-			ret = 0;
-			*enabled = intel_dig_port->audio_enabled;
-			if (!*enabled)
-				break;
+		*enabled = intel_dig_port->audio_enabled;
+		if (*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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] ALSA: hda - Split ELD update code from hdmi_present_sense()

[Takashi Iwai]

This is a preliminary patch for the later change to support ELD/jack
handling with i915 audio component.  This splits the ELD update code
from hdmi_present_sense() so that it can be called from other places.

Just a code refactoring, no functional change.

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

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 4b6fb668c91c..28684aa86408 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1530,6 +1530,56 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 	return 0;
 }
 
+/* update per_pin ELD from the given new ELD;
+ * setup info frame and notification accordingly
+ */
+static void update_eld(struct hda_codec *codec,
+		       struct hdmi_spec_per_pin *per_pin,
+		       struct hdmi_eld *eld)
+{
+	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
+	bool old_eld_valid = pin_eld->eld_valid;
+	bool eld_changed;
+
+	if (eld->eld_valid)
+		snd_hdmi_show_eld(codec, &eld->info);
+
+	eld_changed = (pin_eld->eld_valid != eld->eld_valid);
+	if (eld->eld_valid && pin_eld->eld_valid)
+		if (pin_eld->eld_size != eld->eld_size ||
+		    memcmp(pin_eld->eld_buffer, eld->eld_buffer,
+			   eld->eld_size) != 0)
+			eld_changed = true;
+
+	pin_eld->eld_valid = eld->eld_valid;
+	pin_eld->eld_size = eld->eld_size;
+	if (eld->eld_valid)
+		memcpy(pin_eld->eld_buffer, eld->eld_buffer, eld->eld_size);
+	pin_eld->info = eld->info;
+
+	/*
+	 * Re-setup pin and infoframe. This is needed e.g. when
+	 * - sink is first plugged-in
+	 * - transcoder can change during stream playback on Haswell
+	 *   and this can make HW reset converter selection on a pin.
+	 */
+	if (eld->eld_valid && !old_eld_valid && per_pin->setup) {
+		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
+			intel_verify_pin_cvt_connect(codec, per_pin);
+			intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
+						     per_pin->mux_idx);
+		}
+
+		hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
+	}
+
+	if (eld_changed)
+		snd_ctl_notify(codec->card,
+			       SNDRV_CTL_EVENT_MASK_VALUE |
+			       SNDRV_CTL_EVENT_MASK_INFO,
+			       &per_pin->eld_ctl->id);
+}
+
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 {
 	struct hda_jack_tbl *jack;
@@ -1547,8 +1597,6 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present;
-	bool update_eld = false;
-	bool eld_changed = false;
 	bool ret;
 
 	snd_hda_power_up_pm(codec);
@@ -1575,61 +1623,13 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 						    eld->eld_size) < 0)
 				eld->eld_valid = false;
 		}
-
-		if (eld->eld_valid) {
-			snd_hdmi_show_eld(codec, &eld->info);
-			update_eld = true;
-		}
-		else if (repoll) {
-			schedule_delayed_work(&per_pin->work,
-					      msecs_to_jiffies(300));
-			goto unlock;
-		}
 	}
 
-	if (pin_eld->eld_valid != eld->eld_valid)
-		eld_changed = true;
-
-	if (pin_eld->eld_valid && !eld->eld_valid)
-		update_eld = true;
-
-	if (update_eld) {
-		bool old_eld_valid = pin_eld->eld_valid;
-		pin_eld->eld_valid = eld->eld_valid;
-		if (pin_eld->eld_size != eld->eld_size ||
-			      memcmp(pin_eld->eld_buffer, eld->eld_buffer,
-				     eld->eld_size) != 0) {
-			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
-			       eld->eld_size);
-			eld_changed = true;
-		}
-		pin_eld->eld_size = eld->eld_size;
-		pin_eld->info = eld->info;
-
-		/*
-		 * Re-setup pin and infoframe. This is needed e.g. when
-		 * - sink is first plugged-in (infoframe is not set up if !monitor_present)
-		 * - transcoder can change during stream playback on Haswell
-		 *   and this can make HW reset converter selection on a pin.
-		 */
-		if (eld->eld_valid && !old_eld_valid && per_pin->setup) {
-			if (is_haswell_plus(codec) ||
-				is_valleyview_plus(codec)) {
-				intel_verify_pin_cvt_connect(codec, per_pin);
-				intel_not_share_assigned_cvt(codec, pin_nid,
-							per_pin->mux_idx);
-			}
-
-			hdmi_setup_audio_infoframe(codec, per_pin,
-						   per_pin->non_pcm);
-		}
-	}
+	if (!eld->eld_valid && repoll)
+		schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
+	else
+		update_eld(codec, per_pin, eld);
 
-	if (eld_changed)
-		snd_ctl_notify(codec->card,
-			       SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,
-			       &per_pin->eld_ctl->id);
- unlock:
 	ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid;
 
 	jack = snd_hda_jack_tbl_get(codec, pin_nid);
-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 28684aa86408..8378c31e0b4f 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
@@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec,
 			       &per_pin->eld_ctl->id);
 }
 
+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;
@@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	int present;
 	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,68 @@ 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_spec *spec = codec->spec;
+	struct hdmi_eld *eld = &spec->temp_eld;
+	int size;
+
+	if (acomp && acomp->ops && acomp->ops->get_eld) {
+		mutex_lock(&per_pin->lock);
+		size = acomp->ops->get_eld(acomp->dev,
+					   intel_pin2port(per_pin->pin_nid),
+					   &eld->monitor_present,
+					   eld->eld_buffer,
+					   ELD_MAX_SIZE);
+		if (size > 0) {
+			memset(&eld->info, 0, sizeof(eld->info));
+			if (snd_hdmi_parse_eld(codec, &eld->info,
+					       eld->eld_buffer, size) < 0)
+				size = -EINVAL;
+		}
+
+		if (size > 0) {
+			eld->eld_valid = true;
+			eld->eld_size = size;
+		} else {
+			eld->eld_valid = false;
+			eld->eld_size = 0;
+		}
+
+		update_eld(codec, per_pin, eld);
+		snd_jack_report(per_pin->acomp_jack,
+				eld->monitor_present ? SND_JACK_AVOUT : 0);
+		mutex_unlock(&per_pin->lock);
+	}
+}
+
+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 +2173,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 +2270,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 +2295,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 +2303,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);
@@ -2388,7 +2466,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 			is_broxton(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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] ALSA: hda - Do zero-clear in snd_hdmi_parse_eld() itself

[Takashi Iwai]

Instead of doing in each caller side, snd_hdmi_parse_eld() does
zero-clear of the parsed data by itself.  This is safer and simplifies
the code.

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

diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
index 563984dd2562..bc2e08257c2e 100644
--- a/sound/pci/hda/hda_eld.c
+++ b/sound/pci/hda/hda_eld.c
@@ -253,6 +253,7 @@ int snd_hdmi_parse_eld(struct hda_codec *codec, struct parsed_hdmi_eld *e,
 	int mnl;
 	int i;
 
+	memset(e, 0, sizeof(*e));
 	e->eld_ver = GRAB_BITS(buf, 0, 3, 5);
 	if (e->eld_ver != ELD_VER_CEA_861D &&
 	    e->eld_ver != ELD_VER_PARTIAL) {
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 8378c31e0b4f..23a4292c355f 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1628,7 +1628,6 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 						     &eld->eld_size) < 0)
 			eld->eld_valid = false;
 		else {
-			memset(&eld->info, 0, sizeof(struct parsed_hdmi_eld));
 			if (snd_hdmi_parse_eld(codec, &eld->info, eld->eld_buffer,
 						    eld->eld_size) < 0)
 				eld->eld_valid = false;
@@ -2118,7 +2117,6 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 					   eld->eld_buffer,
 					   ELD_MAX_SIZE);
 		if (size > 0) {
-			memset(&eld->info, 0, sizeof(eld->info));
 			if (snd_hdmi_parse_eld(codec, &eld->info,
 					       eld->eld_buffer, size) < 0)
 				size = -EINVAL;
-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] ALSA: hda - Skip ELD notification during PM process

[Takashi Iwai]

The ELD notification can be received asynchronously from the graphics
side, and this may happen just at the moment the sound driver is
processing the suspend or the resume, and it would confuse the whole
procedure.  Since the ELD and connection states are updated in anyway
at the end of the resume, we can skip it when received during PM
process.

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

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 23a4292c355f..0492f3cf744e 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2433,6 +2433,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port)
 	 */
 	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
 		return;
+	/* ditto during suspend/resume process itself */
+	if (atomic_read(&(codec)->core.in_pm))
+		return;
 
 	check_presence_and_report(codec, pin_nid);
 }
-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> 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.
> It returns the size of ELD bytes if it's valid, or zero if the audio
> is disabled or unplugged, 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 | 40 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  include/drm/i915_component.h       |  3 +++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 0c38cc6c82ae..6b318a8d5dc9 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);
>  
> @@ -702,6 +704,43 @@ 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,
> +					bool *enabled,
> +					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;
> +

Locking is a bit in-flux atm with atomic, but needs
dev_priv->dev->mode_config->mutex here to protect against the eld
changing.

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

MST? Not that I have a clue how that should work ;-)

> +			continue;
> +		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> +		if (port == intel_dig_port->port) {
> +			ret = 0;
> +			*enabled = intel_dig_port->audio_enabled;
> +			if (!*enabled)
> +				break;
> +			connector = drm_select_eld(&intel_encoder->base);
> +			if (!connector)
> +				break;
> +			eld = connector->eld;
> +			ret = min(max_bytes, drm_eld_size(eld));

How can the caller figure out what the eld size is if you use min here? At
least in drm we just return the size we want if it's too small.

> +			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,
> @@ -709,6 +748,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;

Needs a comment that this is protected by dev_priv->av_mutex.

>  	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..058d39e8d57f 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -38,6 +38,7 @@
>   * @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 the audio state and ELD bytes for the given port

In 4.4 kerneldoc supports extended in-line comments for struct members
like this:

>   */
>  struct i915_audio_component_ops {
>  	struct module *owner;
> @@ -46,6 +47,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);

	/**
	 * @get_eld:
	 *
	 * lots of blabla, possibly in multiple paragraphs.
	 */

Please use that layout and put a copy of the more detailed description you
put into the commit message of how ->get_eld exactly works.

I did ask for this to get done as part of some of the previous, and it was
partially done but not exactly how kerneldoc wants it :( But I think we
need to start somewhere ...

Cheers, Daniel


> +	int (*get_eld)(struct device *, int port, bool *enabled,
> +		       unsigned char *buf, int max_bytes);
>  };
>  
>  struct i915_audio_component_audio_ops {
> -- 
> 2.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: refactoring audio component functions

[Daniel Vetter]

On Mon, Nov 30, 2015 at 02:37:47PM +0100, Takashi Iwai wrote:
> 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 | 61 ++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 6b318a8d5dc9..024e88ae6305 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)

If this is static, maybe just maintain a snd_pin_to_port mapping in
dev_priv? That's the approach we're usually taking, and it has the benefit
of making it clear(er) that the binding is static ... Or is it not?

Makes sense otherwise.
-Daniel

> +			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;
> @@ -717,23 +725,18 @@ 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) {
> -			ret = 0;
> -			*enabled = intel_dig_port->audio_enabled;
> -			if (!*enabled)
> -				break;
> +		*enabled = intel_dig_port->audio_enabled;
> +		if (*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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > index 30d89e0da2c6..058d39e8d57f 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -38,6 +38,7 @@
> >   * @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 the audio state and ELD bytes for the given port
> 
> In 4.4 kerneldoc supports extended in-line comments for struct members
> like this:
> 
> >   */
> >  struct i915_audio_component_ops {
> >  	struct module *owner;
> > @@ -46,6 +47,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);
> 
> 	/**
> 	 * @get_eld:
> 	 *
> 	 * lots of blabla, possibly in multiple paragraphs.
> 	 */
> 
> Please use that layout and put a copy of the more detailed description you
> put into the commit message of how ->get_eld exactly works.
> 
> I did ask for this to get done as part of some of the previous, and it was
> partially done but not exactly how kerneldoc wants it :( But I think we
> need to start somewhere ...

Strike that, I looked at the wrong tree ;-) linux-next should have all the
patches using the new extended style.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Mon, 30 Nov 2015 15:11:16 +0100,
Daniel Vetter wrote:
> 
> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > 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.
> > It returns the size of ELD bytes if it's valid, or zero if the audio
> > is disabled or unplugged, 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 | 40 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  include/drm/i915_component.h       |  3 +++
> >  3 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 0c38cc6c82ae..6b318a8d5dc9 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);
> >  
> > @@ -702,6 +704,43 @@ 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,
> > +					bool *enabled,
> > +					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;
> > +
> 
> Locking is a bit in-flux atm with atomic, but needs
> dev_priv->dev->mode_config->mutex here to protect against the eld
> changing.

Noted, I'll add it in the next respin.

> > +	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)
> 
> MST? Not that I have a clue how that should work ;-)

Well, I supposed that MST entry doesn't actually serve for the digital
port, but I'm not entirely sure.  In anyway, the whole MST audio
support shall be added / handled by other Intel people soon later, so
let's keep this as is -- which is the same condition as the current
i915 code.

> > +			continue;
> > +		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> > +		if (port == intel_dig_port->port) {
> > +			ret = 0;
> > +			*enabled = intel_dig_port->audio_enabled;
> > +			if (!*enabled)
> > +				break;
> > +			connector = drm_select_eld(&intel_encoder->base);
> > +			if (!connector)
> > +				break;
> > +			eld = connector->eld;
> > +			ret = min(max_bytes, drm_eld_size(eld));
> 
> How can the caller figure out what the eld size is if you use min here? At
> least in drm we just return the size we want if it's too small.

A good point.  The function should return the size "to be written
all", indeed.

> > +			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,
> > @@ -709,6 +748,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;
> 
> Needs a comment that this is protected by dev_priv->av_mutex.

OK.

> >  	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..058d39e8d57f 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -38,6 +38,7 @@
> >   * @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 the audio state and ELD bytes for the given port
> 
> In 4.4 kerneldoc supports extended in-line comments for struct members
> like this:
> 
> >   */
> >  struct i915_audio_component_ops {
> >  	struct module *owner;
> > @@ -46,6 +47,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);
> 
> 	/**
> 	 * @get_eld:
> 	 *
> 	 * lots of blabla, possibly in multiple paragraphs.
> 	 */
> 
> Please use that layout and put a copy of the more detailed description you
> put into the commit message of how ->get_eld exactly works.
> 
> I did ask for this to get done as part of some of the previous, and it was
> partially done but not exactly how kerneldoc wants it :( But I think we
> need to start somewhere ...

Yeah, it looks like that I missed the back-merge of 4.4-rc although I
thought I did.  Will rebase and rewrite that.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: refactoring audio component functions

[Takashi Iwai]

On Mon, 30 Nov 2015 15:14:03 +0100,
Daniel Vetter wrote:
> 
> On Mon, Nov 30, 2015 at 02:37:47PM +0100, Takashi Iwai wrote:
> > 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 | 61 ++++++++++++++++++++------------------
> >  1 file changed, 32 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 6b318a8d5dc9..024e88ae6305 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)
> 
> If this is static, maybe just maintain a snd_pin_to_port mapping in
> dev_priv? That's the approach we're usually taking, and it has the benefit
> of making it clear(er) that the binding is static ... Or is it not?

Yes, I *guess* this is static, but need a double-check.  The current
patchset just derives from the existing code.


thanks,

Takashi

> Makes sense otherwise.
> -Daniel
> 
> > +			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;
> > @@ -717,23 +725,18 @@ 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) {
> > -			ret = 0;
> > -			*enabled = intel_dig_port->audio_enabled;
> > -			if (!*enabled)
> > -				break;
> > +		*enabled = intel_dig_port->audio_enabled;
> > +		if (*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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Ville Syrjälä]

On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> 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.
> It returns the size of ELD bytes if it's valid, or zero if the audio
> is disabled or unplugged, or a negative error code.

Why do we need this? Isn't it something the eld notify hook should
pass from i915 to the audio driver?

At least with the locking you have for this, the audio driver can not
call this from the eld notify hook since it would deadlock.

> 
> 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 | 40 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  include/drm/i915_component.h       |  3 +++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 0c38cc6c82ae..6b318a8d5dc9 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);
>  
> @@ -702,6 +704,43 @@ 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,
> +					bool *enabled,
> +					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) {
> +			ret = 0;
> +			*enabled = intel_dig_port->audio_enabled;
> +			if (!*enabled)
> +				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,
> @@ -709,6 +748,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..058d39e8d57f 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -38,6 +38,7 @@
>   * @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 the audio state and ELD bytes for the given port
>   */
>  struct i915_audio_component_ops {
>  	struct module *owner;
> @@ -46,6 +47,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, bool *enabled,
> +		       unsigned char *buf, int max_bytes);
>  };
>  
>  struct i915_audio_component_audio_ops {
> -- 
> 2.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Mon, 30 Nov 2015 16:24:41 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > 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.
> > It returns the size of ELD bytes if it's valid, or zero if the audio
> > is disabled or unplugged, or a negative error code.
> 
> Why do we need this? Isn't it something the eld notify hook should
> pass from i915 to the audio driver?

We need this at least in two situations:
- when the audio driver is probed
- when the audio driver is resumed

> At least with the locking you have for this, the audio driver can not
> call this from the eld notify hook since it would deadlock.

OK, then we may change the eld_notify to pass the values in the
arguments, and also add the new op with a lock to be called from other
places from other places.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[David Henningsson]

On 2015-11-30 16:29, Takashi Iwai wrote:
> On Mon, 30 Nov 2015 16:24:41 +0100,
> Ville Syrjälä wrote:
>>
>> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
>>> 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.
>>> It returns the size of ELD bytes if it's valid, or zero if the audio
>>> is disabled or unplugged, or a negative error code.
>>
>> Why do we need this? Isn't it something the eld notify hook should
>> pass from i915 to the audio driver?
>
> We need this at least in two situations:
> - when the audio driver is probed
> - when the audio driver is resumed
>
>> At least with the locking you have for this, the audio driver can not
>> call this from the eld notify hook since it would deadlock.
>
> OK, then we may change the eld_notify to pass the values in the
> arguments, and also add the new op with a lock to be called from other
> places from other places.

Maybe we have to make eld_notify not do anything except to call 
schedule_work then? And that work in turn will ask for updated eld from 
the i915 driver, and handle the result.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Mon, 30 Nov 2015 16:34:22 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-11-30 16:29, Takashi Iwai wrote:
> > On Mon, 30 Nov 2015 16:24:41 +0100,
> > Ville Syrjälä wrote:
> >>
> >> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> >>> 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.
> >>> It returns the size of ELD bytes if it's valid, or zero if the audio
> >>> is disabled or unplugged, or a negative error code.
> >>
> >> Why do we need this? Isn't it something the eld notify hook should
> >> pass from i915 to the audio driver?
> >
> > We need this at least in two situations:
> > - when the audio driver is probed
> > - when the audio driver is resumed
> >
> >> At least with the locking you have for this, the audio driver can not
> >> call this from the eld notify hook since it would deadlock.
> >
> > OK, then we may change the eld_notify to pass the values in the
> > arguments, and also add the new op with a lock to be called from other
> > places from other places.
> 
> Maybe we have to make eld_notify not do anything except to call 
> schedule_work then? And that work in turn will ask for updated eld from 
> the i915 driver, and handle the result.

Yes, it would work, too -- though, this would need a bit more code
reorganization.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Mon, 30 Nov 2015 15:17:05 +0100,
Daniel Vetter wrote:
> 
> On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > index 30d89e0da2c6..058d39e8d57f 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -38,6 +38,7 @@
> > >   * @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 the audio state and ELD bytes for the given port
> > 
> > In 4.4 kerneldoc supports extended in-line comments for struct members
> > like this:
> > 
> > >   */
> > >  struct i915_audio_component_ops {
> > >  	struct module *owner;
> > > @@ -46,6 +47,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);
> > 
> > 	/**
> > 	 * @get_eld:
> > 	 *
> > 	 * lots of blabla, possibly in multiple paragraphs.
> > 	 */
> > 
> > Please use that layout and put a copy of the more detailed description you
> > put into the commit message of how ->get_eld exactly works.
> > 
> > I did ask for this to get done as part of some of the previous, and it was
> > partially done but not exactly how kerneldoc wants it :( But I think we
> > need to start somewhere ...
> 
> Strike that, I looked at the wrong tree ;-) linux-next should have all the
> patches using the new extended style.

OK, so this is a post-4.4 change.  I can rebase on it.  Could you
point a steady branch suitable for it?


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] ALSA: hda - Split ELD update code from hdmi_present_sense()

[Vinod Koul]

On Mon, Nov 30, 2015 at 02:37:48PM +0100, Takashi Iwai wrote:
>  
> +/* update per_pin ELD from the given new ELD;
> + * setup info frame and notification accordingly
> + */

nitpick, comment style

-- 
~Vinod
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] ALSA: hda - Split ELD update code from hdmi_present_sense()

[Takashi Iwai]

On Mon, 30 Nov 2015 17:00:33 +0100,
Vinod Koul wrote:
> 
> On Mon, Nov 30, 2015 at 02:37:48PM +0100, Takashi Iwai wrote:
> >  
> > +/* update per_pin ELD from the given new ELD;
> > + * setup info frame and notification accordingly
> > + */
> 
> nitpick, comment style

There is no preference in sound/* about this, and I myself prefer
without the extra line :)


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Ville Syrjälä]

On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > 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.
> > It returns the size of ELD bytes if it's valid, or zero if the audio
> > is disabled or unplugged, or a negative error code.
> 
> Why do we need this? Isn't it something the eld notify hook should
> pass from i915 to the audio driver?
> 
> At least with the locking you have for this, the audio driver can not
> call this from the eld notify hook since it would deadlock.

Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
the mode_config.mutex will make it so.

Apart from that it seesm to me that you should pull the av_mutex
lock/unlock from the .audio_code_eanble/disable hooks into
intel_audio_codec_enable/disable, so that it protects the audio_enabled
flag as well. Not sure if the eld_notify should be called while holding
that lock or not. If we need to avoid calling it from the eld_notify
anywya due to other locks then maybe it can be under av_mutex as well.

> 
> > 
> > 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 | 40 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  include/drm/i915_component.h       |  3 +++
> >  3 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 0c38cc6c82ae..6b318a8d5dc9 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);
> >  
> > @@ -702,6 +704,43 @@ 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,
> > +					bool *enabled,
> > +					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) {
> > +			ret = 0;
> > +			*enabled = intel_dig_port->audio_enabled;
> > +			if (!*enabled)
> > +				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,
> > @@ -709,6 +748,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..058d39e8d57f 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -38,6 +38,7 @@
> >   * @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 the audio state and ELD bytes for the given port
> >   */
> >  struct i915_audio_component_ops {
> >  	struct module *owner;
> > @@ -46,6 +47,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, bool *enabled,
> > +		       unsigned char *buf, int max_bytes);
> >  };
> >  
> >  struct i915_audio_component_audio_ops {
> > -- 
> > 2.6.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:55:50PM +0100, Takashi Iwai wrote:
> On Mon, 30 Nov 2015 15:17:05 +0100,
> Daniel Vetter wrote:
> > 
> > On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > > index 30d89e0da2c6..058d39e8d57f 100644
> > > > --- a/include/drm/i915_component.h
> > > > +++ b/include/drm/i915_component.h
> > > > @@ -38,6 +38,7 @@
> > > >   * @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 the audio state and ELD bytes for the given port
> > > 
> > > In 4.4 kerneldoc supports extended in-line comments for struct members
> > > like this:
> > > 
> > > >   */
> > > >  struct i915_audio_component_ops {
> > > >  	struct module *owner;
> > > > @@ -46,6 +47,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);
> > > 
> > > 	/**
> > > 	 * @get_eld:
> > > 	 *
> > > 	 * lots of blabla, possibly in multiple paragraphs.
> > > 	 */
> > > 
> > > Please use that layout and put a copy of the more detailed description you
> > > put into the commit message of how ->get_eld exactly works.
> > > 
> > > I did ask for this to get done as part of some of the previous, and it was
> > > partially done but not exactly how kerneldoc wants it :( But I think we
> > > need to start somewhere ...
> > 
> > Strike that, I looked at the wrong tree ;-) linux-next should have all the
> > patches using the new extended style.
> 
> OK, so this is a post-4.4 change.  I can rebase on it.  Could you
> point a steady branch suitable for it?

Dave's drm-next would be it, if Dave opens it up and pulls in the 2 pull
requests I've sent out earlier. Dave?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > 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.
> > > It returns the size of ELD bytes if it's valid, or zero if the audio
> > > is disabled or unplugged, or a negative error code.
> > 
> > Why do we need this? Isn't it something the eld notify hook should
> > pass from i915 to the audio driver?
> > 
> > At least with the locking you have for this, the audio driver can not
> > call this from the eld notify hook since it would deadlock.
> 
> Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
> the mode_config.mutex will make it so.

If we need this we could create a substruct in dev_priv with copies of
everything, which would only be protected by av_mutex. That's the usual
approach we use when faced with this kind of locking inversion:
Copy/update relevant data in the modeset ->enable/disable hooks, then just
use these local locks to access those copies. Usually that's enough to
untangle things, with no need to resort to workers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vinod Koul]

On Mon, Nov 30, 2015 at 02:37:49PM +0100, 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 28684aa86408..8378c31e0b4f 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
> @@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec,
>  			       &per_pin->eld_ctl->id);
>  }
>  
> +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;
> @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  	int present;
>  	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,68 @@ 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_spec *spec = codec->spec;
> +	struct hdmi_eld *eld = &spec->temp_eld;
> +	int size;
> +
> +	if (acomp && acomp->ops && acomp->ops->get_eld) {
> +		mutex_lock(&per_pin->lock);
> +		size = acomp->ops->get_eld(acomp->dev,
> +					   intel_pin2port(per_pin->pin_nid),
> +					   &eld->monitor_present,
> +					   eld->eld_buffer,
> +					   ELD_MAX_SIZE);
> +		if (size > 0) {
> +			memset(&eld->info, 0, sizeof(eld->info));
> +			if (snd_hdmi_parse_eld(codec, &eld->info,
> +					       eld->eld_buffer, size) < 0)
> +				size = -EINVAL;
> +		}
> +
> +		if (size > 0) {
> +			eld->eld_valid = true;
> +			eld->eld_size = size;
> +		} else {
> +			eld->eld_valid = false;
> +			eld->eld_size = 0;
> +		}
> +
> +		update_eld(codec, per_pin, eld);
> +		snd_jack_report(per_pin->acomp_jack,
> +				eld->monitor_present ? SND_JACK_AVOUT : 0);
> +		mutex_unlock(&per_pin->lock);
> +	}
> +}

IMO This and the rest can be moved to sound/hda/ so that other can reuse
this code, as the code will be same for other users too..

-- 
~Vinod
> +
> +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 +2173,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 +2270,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 +2295,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 +2303,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);
> @@ -2388,7 +2466,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
>  			is_broxton(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
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Mon, 30 Nov 2015 17:42:33 +0100,
Vinod Koul wrote:
> 
> On Mon, Nov 30, 2015 at 02:37:49PM +0100, 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 82 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 28684aa86408..8378c31e0b4f 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
> > @@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec,
> >  			       &per_pin->eld_ctl->id);
> >  }
> >  
> > +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;
> > @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >  	int present;
> >  	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,68 @@ 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_spec *spec = codec->spec;
> > +	struct hdmi_eld *eld = &spec->temp_eld;
> > +	int size;
> > +
> > +	if (acomp && acomp->ops && acomp->ops->get_eld) {
> > +		mutex_lock(&per_pin->lock);
> > +		size = acomp->ops->get_eld(acomp->dev,
> > +					   intel_pin2port(per_pin->pin_nid),
> > +					   &eld->monitor_present,
> > +					   eld->eld_buffer,
> > +					   ELD_MAX_SIZE);
> > +		if (size > 0) {
> > +			memset(&eld->info, 0, sizeof(eld->info));
> > +			if (snd_hdmi_parse_eld(codec, &eld->info,
> > +					       eld->eld_buffer, size) < 0)
> > +				size = -EINVAL;
> > +		}
> > +
> > +		if (size > 0) {
> > +			eld->eld_valid = true;
> > +			eld->eld_size = size;
> > +		} else {
> > +			eld->eld_valid = false;
> > +			eld->eld_size = 0;
> > +		}
> > +
> > +		update_eld(codec, per_pin, eld);
> > +		snd_jack_report(per_pin->acomp_jack,
> > +				eld->monitor_present ? SND_JACK_AVOUT : 0);
> > +		mutex_unlock(&per_pin->lock);
> > +	}
> > +}
> 
> IMO This and the rest can be moved to sound/hda/ so that other can reuse
> this code, as the code will be same for other users too..

We have no per_pin or HDMI specific stuff there yet.
What we can reuse at most is only the acomp->ops... call itself, so
far.  hdac_i915.c is a thin wrapper, and it doesn't handle anything
deep wrt HDMI/DP by itself.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Ville Syrjälä]

On Mon, Nov 30, 2015 at 05:34:45PM +0100, Daniel Vetter wrote:
> On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > > 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.
> > > > It returns the size of ELD bytes if it's valid, or zero if the audio
> > > > is disabled or unplugged, or a negative error code.
> > > 
> > > Why do we need this? Isn't it something the eld notify hook should
> > > pass from i915 to the audio driver?
> > > 
> > > At least with the locking you have for this, the audio driver can not
> > > call this from the eld notify hook since it would deadlock.
> > 
> > Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
> > the mode_config.mutex will make it so.
> 
> If we need this we could create a substruct in dev_priv with copies of
> everything, which would only be protected by av_mutex. That's the usual
> approach we use when faced with this kind of locking inversion:
> Copy/update relevant data in the modeset ->enable/disable hooks, then just
> use these local locks to access those copies. Usually that's enough to
> untangle things, with no need to resort to workers.

Yeah, IIRC I suggested just that originally.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Mon, 30 Nov 2015 17:09:33 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > 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.
> > > It returns the size of ELD bytes if it's valid, or zero if the audio
> > > is disabled or unplugged, or a negative error code.
> > 
> > Why do we need this? Isn't it something the eld notify hook should
> > pass from i915 to the audio driver?
> > 
> > At least with the locking you have for this, the audio driver can not
> > call this from the eld notify hook since it would deadlock.
> 
> Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
> the mode_config.mutex will make it so.
> 
> Apart from that it seesm to me that you should pull the av_mutex
> lock/unlock from the .audio_code_eanble/disable hooks into
> intel_audio_codec_enable/disable, so that it protects the audio_enabled
> flag as well. Not sure if the eld_notify should be called while holding
> that lock or not. If we need to avoid calling it from the eld_notify
> anywya due to other locks then maybe it can be under av_mutex as well.

Currently I'm thinking of:

- not allow to call get_eld directly from eld_notify;
  I found that the existing eld_repoll work in the HDA can be reused
  easily, so let's follow Daniel's advice.

- drm_select_eld() seems requring mode_config.mutex and
  connection_mutex modeset lock in anyway; so let get_eld taking
  both.

  Is it OK to call drm_modeset_lock_all() for simplicity?

- Get rid of av_mutex from get_eld instead;
  get_eld doesn't conflict with other ops

In that way, audio_enabled flag is protected in both places via
modeset lock, I suppose.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx