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

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

[Takashi Iwai]

Hi,

this is a revised 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 patch still doesn't follow the recent kernel doc comment changes
in i915_component.h.  I'll rebase once when I get the steady branch in
drm tree.

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


Takashi

===

v1->v2: 
* Use modeset lock for get_eld lock, drop av mutex
* Return the expected size from get_eld, not the copied size
* Add reverse map from a port number to the encoder
* Deferred invocation of get_eld from the eld_notify in HDA side
* A bit more code refactoring in HDA side
* Move audio component accessors to hdac_i915.c


Takashi Iwai (9):
  drm/i915: Remove superfluous NULL check
  drm/i915: Add get_eld audio component
  drm/i915: refactoring audio component functions
  drm/i915: Add reverse mapping between port and intel_encoder
  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
  ALSA: hda - Move audio component accesses to hdac_i915.c

 drivers/gpu/drm/i915/i915_drv.h    |   2 +
 drivers/gpu/drm/i915/intel_audio.c |  59 ++++++---
 drivers/gpu/drm/i915/intel_ddi.c   |   1 +
 drivers/gpu/drm/i915/intel_dp.c    |   1 +
 drivers/gpu/drm/i915/intel_drv.h   |   1 +
 drivers/gpu/drm/i915/intel_hdmi.c  |   2 +
 include/drm/i915_component.h       |   6 +
 include/sound/hda_i915.h           |  14 +++
 sound/hda/hdac_i915.c              |  66 +++++++++++
 sound/pci/hda/hda_eld.c            |   1 +
 sound/pci/hda/patch_hdmi.c         | 237 +++++++++++++++++++++++++------------
 11 files changed, 294 insertions(+), 96 deletions(-)

-- 
2.6.3

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

[PATCH v2 1/9] 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 v2 2/9] 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 audio status and ELD of the
given HDMI/DP port.  It returns the size of expected ELD bytes if it's
valid, zero if no valid ELD is found, or a negative error code.  The
current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes.  If
the size is greater than the max bytes, it means that only a part of
ELD has been copied back.

A big warning about the usage of this callback is: you must not call
it from eld_notify.  The eld_notify itself is called in the modeset
lock, and it leads to a deadlock since get_eld takes the modeset lock,
too.  You need to call get_eld in a work, for example, in such a case.
We'll see the actual implementation in the later patch in
sound/pci/hda/patch_hdmi.c.

For achieving this implementation, 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.  It's protected with the modeset
lock as well.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2:
* Use modeset lock for get_eld lock, drop av mutex
* Return the expected size from get_eld, not the copied size

 drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 include/drm/i915_component.h       |  6 ++++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 0c38cc6c82ae..1965a61769ea 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;
+
+	drm_modeset_lock_all(drm_dev);
+	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 = drm_eld_size(eld);
+			memcpy(buf, eld, min(max_bytes, ret));
+			break;
+		}
+	}
+
+	drm_modeset_unlock_all(drm_dev);
+	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..013779db7d24 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 {
@@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
 	 * pin sense and/or ELD information has changed.
 	 * @audio_ptr:		HDA driver object
 	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
+	 *
+	 * Note that you can't call i915_audio_component_ops.get_eld directly
+	 * from the notifier callback as it may lead to deadlocks.
 	 */
 	void (*pin_eld_notify)(void *audio_ptr, int port);
 };
-- 
2.6.3

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

[PATCH v2 3/9] 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 1965a61769ea..19958bdb9bd0 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;
 
 	drm_modeset_lock_all(drm_dev);
-	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 = drm_eld_size(eld);
-			memcpy(buf, eld, min(max_bytes, ret));
-			break;
+			if (connector) {
+				eld = connector->eld;
+				ret = drm_eld_size(eld);
+				memcpy(buf, eld, min(max_bytes, ret));
+			}
 		}
 	}
 
-- 
2.6.3

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

[PATCH v2 4/9] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

This patch adds a reverse mapping from a digital port number to
intel_encoder object containing the corresponding intel_digital_port.
It simplifies the query of the encoder a lot.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_audio.c | 22 ++--------------------
 drivers/gpu/drm/i915/intel_ddi.c   |  1 +
 drivers/gpu/drm/i915/intel_dp.c    |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c  |  2 ++
 5 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 95bb27de774f..3483d8125eac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1955,6 +1955,8 @@ struct drm_i915_private {
 	/* perform PHY state sanity checks? */
 	bool chv_phy_assert[2];
 
+	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 19958bdb9bd0..67ee99f00ddd 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -630,28 +630,10 @@ 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_crtc *crtc;
 	struct drm_display_mode *mode;
@@ -668,7 +650,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 
 	mutex_lock(&dev_priv->av_mutex);
 	/* 1. get the pipe */
-	intel_encoder = audio_port_to_encoder(drm_dev, port);
+	intel_encoder = dev_priv->dig_port_map[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);
@@ -725,7 +707,7 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 	int ret = -EINVAL;
 
 	drm_modeset_lock_all(drm_dev);
-	intel_encoder = audio_port_to_encoder(drm_dev, port);
+	intel_encoder = dev_priv->dig_port_map[port];
 	if (intel_encoder) {
 		ret = 0;
 		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a6752a61d99f..6770110a4075 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3285,6 +3285,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->get_config = intel_ddi_get_config;
 
 	intel_dig_port->port = port;
+	dev_priv->dig_port_map[port] = intel_encoder;
 	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
 					  (DDI_BUF_PORT_REVERSAL |
 					   DDI_A_4_LANES);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 09bdd94ca3ba..1c02c6466f30 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6201,6 +6201,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	}
 
 	intel_dig_port->port = port;
+	dev_priv->dig_port_map[port] = intel_encoder;
 	intel_dig_port->dp.output_reg = output_reg;
 
 	intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9eafa191cee2..1d9f522a6679 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2133,6 +2133,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_digital_port *intel_dig_port;
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
@@ -2201,6 +2202,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->cloneable |= 1 << INTEL_OUTPUT_HDMI;
 
 	intel_dig_port->port = port;
+	dev_priv->dig_port_map[port] = intel_encoder;
 	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
 	intel_dig_port->dp.output_reg = 0;
 
-- 
2.6.3

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

[PATCH v2 5/9] 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 v2 6/9] 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.

In the eld_notify callback, the execution of hdmi_present_sense() is
done in a work for avoiding the deadlock in drm side.  It's reusing
the existing hdmi_repoll_eld() but ignoring the repoll_count (with the
direct access to the graphics driver, we need really no repoll, in
anyway).

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

v1->v2:
* Deferred invocation of get_eld from the eld_notify in HDA side
* A bit more code refactoring

 sound/pci/hda/patch_hdmi.c | 133 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 116 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 28684aa86408..c90b4d19c64b 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
@@ -1435,6 +1437,17 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
 	}
 }
 
+/* There is a fixed mapping between audio pin node and display port
+ * on current Intel platforms:
+ * Pin Widget 5 - PORT B (port = 1 in i915 driver)
+ * Pin Widget 6 - PORT C (port = 2 in i915 driver)
+ * Pin Widget 7 - PORT D (port = 3 in i915 driver)
+ */
+static int intel_pin2port(hda_nid_t pin_nid)
+{
+	return pin_nid - 4;
+}
+
 /*
  * HDA PCM callbacks
  */
@@ -1580,7 +1593,9 @@ static void update_eld(struct hda_codec *codec,
 			       &per_pin->eld_ctl->id);
 }
 
-static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
+/* update ELD and jack state via HD-audio verbs */
+static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
+					 int repoll)
 {
 	struct hda_jack_tbl *jack;
 	struct hda_codec *codec = per_pin->codec;
@@ -1641,16 +1656,71 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	return ret;
 }
 
+/* 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));
+			size = min(size, ELD_MAX_SIZE);
+			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 bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
+{
+	struct hda_codec *codec = per_pin->codec;
+	struct hdmi_spec *spec = codec->spec;
+
+	if (spec->use_acomp) {
+		sync_eld_via_acomp(codec, per_pin);
+		return false; /* don't call snd_hda_jack_report_sync() */
+	} else {
+		return hdmi_present_sense_via_verbs(per_pin, repoll);
+	}
+}
+
 static void hdmi_repoll_eld(struct work_struct *work)
 {
 	struct hdmi_spec_per_pin *per_pin =
 	container_of(to_delayed_work(work), struct hdmi_spec_per_pin, work);
+	struct hda_codec *codec = per_pin->codec;
+	struct hdmi_spec *spec = codec->spec;
 
-	if (per_pin->repoll_count++ > 6)
+	/* no repoll for audio component; or no more if it's over 6 times */
+	if (spec->use_acomp || per_pin->repoll_count++ > 6)
 		per_pin->repoll_count = 0;
 
 	if (hdmi_present_sense(per_pin, per_pin->repoll_count))
-		snd_hda_jack_report_sync(per_pin->codec);
+		snd_hda_jack_report_sync(codec);
 }
 
 static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
@@ -1776,17 +1846,6 @@ static bool check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
 	return non_pcm;
 }
 
-/* There is a fixed mapping between audio pin node and display port
- * on current Intel platforms:
- * Pin Widget 5 - PORT B (port = 1 in i915 driver)
- * Pin Widget 6 - PORT C (port = 2 in i915 driver)
- * Pin Widget 7 - PORT D (port = 3 in i915 driver)
- */
-static int intel_pin2port(hda_nid_t pin_nid)
-{
-	return pin_nid - 4;
-}
-
 /*
  * HDMI callbacks
  */
@@ -2091,6 +2150,30 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec)
 	return 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 +2184,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 +2281,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 +2306,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 +2314,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);
@@ -2350,7 +2439,13 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 static void intel_pin_eld_notify(void *audio_ptr, int port)
 {
 	struct hda_codec *codec = audio_ptr;
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_nid = port + 0x04;
+	int pin_idx = pin_nid_to_pin_index(codec, pin_nid);
+
+	if (pin_idx < 0)
+		return;
 
 	/* skip notification during system suspend (but not in runtime PM);
 	 * the state will be updated at resume
@@ -2358,7 +2453,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port)
 	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
 		return;
 
-	check_presence_and_report(codec, pin_nid);
+	/* execute ELD update in a work for avoiding a deadlock */
+	per_pin = get_pin(spec, pin_idx);
+	schedule_delayed_work(&per_pin->work, 0);
 }
 
 static int patch_generic_hdmi(struct hda_codec *codec)
@@ -2388,7 +2485,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 v2 7/9] 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 c90b4d19c64b..4dc21ecf7230 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1633,7 +1633,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 						     &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;
@@ -1673,7 +1672,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));
 			size = min(size, ELD_MAX_SIZE);
 			if (snd_hdmi_parse_eld(codec, &eld->info,
 					       eld->eld_buffer, size) < 0)
-- 
2.6.3

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

[PATCH v2 8/9] 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 4dc21ecf7230..75815acb77db 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2450,6 +2450,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;
 
 	/* execute ELD update in a work for avoiding a deadlock */
 	per_pin = get_pin(spec, pin_idx);
-- 
2.6.3

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

[PATCH v2 9/9] ALSA: hda - Move audio component accesses to hdac_i915.c

[Takashi Iwai]

A couple of i915_audio_component ops have been added and accessed
directly from patch_hdmi.c.  Ideally all these should be factored out
into hdac_i915.c.

This patch does it, adds two new helper functions for setting N/CTS
and fetching ELD bytes.  One bonus is that the hackish widget vs port
mapping is also moved to hdac_i915.c, so that it can be fixed /
enhanced more cleanly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hda_i915.h   | 14 ++++++++++
 sound/hda/hdac_i915.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++
 sound/pci/hda/patch_hdmi.c | 69 +++++++++++++++++-----------------------------
 3 files changed, 106 insertions(+), 43 deletions(-)

diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index 930b41e5acf4..fa341fcb5829 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -10,6 +10,9 @@
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
 int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
 int snd_hdac_get_display_clk(struct hdac_bus *bus);
+int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate);
+int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
+			   bool *audio_enabled, char *buffer, int max_bytes);
 int snd_hdac_i915_init(struct hdac_bus *bus);
 int snd_hdac_i915_exit(struct hdac_bus *bus);
 int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *);
@@ -26,6 +29,17 @@ static inline int snd_hdac_get_display_clk(struct hdac_bus *bus)
 {
 	return 0;
 }
+static inline int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid,
+					   int rate)
+{
+	return 0;
+}
+static inline int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
+					 bool *audio_enabled, char *buffer,
+					 int max_bytes)
+{
+	return -ENODEV;
+}
 static inline int snd_hdac_i915_init(struct hdac_bus *bus)
 {
 	return -ENODEV;
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 8fef1b8d1fd8..c50177fb469f 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -118,6 +118,72 @@ int snd_hdac_get_display_clk(struct hdac_bus *bus)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
 
+/* There is a fixed mapping between audio pin node and display port
+ * on current Intel platforms:
+ * Pin Widget 5 - PORT B (port = 1 in i915 driver)
+ * Pin Widget 6 - PORT C (port = 2 in i915 driver)
+ * Pin Widget 7 - PORT D (port = 3 in i915 driver)
+ */
+static int pin2port(hda_nid_t pin_nid)
+{
+	return pin_nid - 4;
+}
+
+/**
+ * snd_hdac_sync_audio_rate - Set N/CTS based on the sample rate
+ * @bus: HDA core bus
+ * @nid: the pin widget NID
+ * @rate: the sample rate to set
+ *
+ * This function is supposed to be used only by a HD-audio controller
+ * driver that needs the interaction with i915 graphics.
+ *
+ * This function sets N/CTS value based on the given sample rate.
+ * Returns zero for success, or a negative error code.
+ */
+int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate)
+{
+	struct i915_audio_component *acomp = bus->audio_component;
+
+	if (!acomp || !acomp->ops || !acomp->ops->sync_audio_rate)
+		return -ENODEV;
+	return acomp->ops->sync_audio_rate(acomp->dev, pin2port(nid), rate);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
+
+/**
+ * snd_hdac_acomp_get_eld - Get the audio state and ELD via component
+ * @bus: HDA core bus
+ * @nid: the pin widget NID
+ * @audio_enabled: the pointer to store the current audio state
+ * @buffer: the buffer pointer to store ELD bytes
+ * @max_bytes: the max bytes to be stored on @buffer
+ *
+ * This function is supposed to be used only by a HD-audio controller
+ * driver that needs the interaction with i915 graphics.
+ *
+ * This function queries the current state of the audio on the given
+ * digital port and fetches the ELD bytes onto the given buffer.
+ * It returns the number of bytes for the total ELD data, zero for
+ * invalid ELD, or a negative error code.
+ *
+ * The return size is the total bytes required for the whole ELD bytes,
+ * thus it may be over @max_bytes.  If it's over @max_bytes, it implies
+ * that only a part of ELD bytes have been fetched.
+ */
+int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
+			   bool *audio_enabled, char *buffer, int max_bytes)
+{
+	struct i915_audio_component *acomp = bus->audio_component;
+
+	if (!acomp || !acomp->ops || !acomp->ops->get_eld)
+		return -ENODEV;
+
+	return acomp->ops->get_eld(acomp->dev, pin2port(nid), audio_enabled,
+				   buffer, max_bytes);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);
+
 static int hdac_component_master_bind(struct device *dev)
 {
 	struct i915_audio_component *acomp = hdac_acomp;
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 75815acb77db..b642191d3da5 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1437,17 +1437,6 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
 	}
 }
 
-/* There is a fixed mapping between audio pin node and display port
- * on current Intel platforms:
- * Pin Widget 5 - PORT B (port = 1 in i915 driver)
- * Pin Widget 6 - PORT C (port = 2 in i915 driver)
- * Pin Widget 7 - PORT D (port = 3 in i915 driver)
- */
-static int intel_pin2port(hda_nid_t pin_nid)
-{
-	return pin_nid - 4;
-}
-
 /*
  * HDA PCM callbacks
  */
@@ -1659,38 +1648,36 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 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) {
-			size = min(size, ELD_MAX_SIZE);
-			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);
+	mutex_lock(&per_pin->lock);
+	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
+				      &eld->monitor_present, eld->eld_buffer,
+				      ELD_MAX_SIZE);
+	if (size < 0)
+		goto unlock;
+	if (size > 0) {
+		size = min(size, ELD_MAX_SIZE);
+		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);
+ unlock:
+	mutex_unlock(&per_pin->lock);
 }
 
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
@@ -1860,7 +1847,6 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 	struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 	hda_nid_t pin_nid = per_pin->pin_nid;
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct i915_audio_component *acomp = codec->bus->core.audio_component;
 	bool non_pcm;
 	int pinctl;
 
@@ -1879,10 +1865,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 
 	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
 	/* Todo: add DP1.2 MST audio support later */
-	if (acomp && acomp->ops && acomp->ops->sync_audio_rate)
-		acomp->ops->sync_audio_rate(acomp->dev,
-				intel_pin2port(pin_nid),
-				runtime->rate);
+	snd_hdac_sync_audio_rate(&codec->bus->core, pin_nid, runtime->rate);
 
 	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
 	mutex_lock(&per_pin->lock);
-- 
2.6.3

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

Re: [PATCH v2 8/9] ALSA: hda - Skip ELD notification during PM process

[Takashi Iwai]

On Tue, 01 Dec 2015 17:09:57 +0100,
Takashi Iwai wrote:
> 
> 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>

I applied this particular patch now to for-next branch, as it's
basically irrelevant with others.

The rest still needs review.


thanks,

Takashi

> ---
>  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 4dc21ecf7230..75815acb77db 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -2450,6 +2450,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;
>  
>  	/* execute ELD update in a work for avoiding a deadlock */
>  	per_pin = get_pin(spec, pin_idx);
> -- 
> 2.6.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> Implement a new i915_audio_component_ops, get_eld().  It's called by
> the audio driver to fetch the current audio status and ELD of the
> given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> valid, zero if no valid ELD is found, or a negative error code.  The
> current state of audio on/off is stored in the given pointer, too.
> 
> Note that the returned size isn't limited to the given max bytes.  If
> the size is greater than the max bytes, it means that only a part of
> ELD has been copied back.
> 
> A big warning about the usage of this callback is: you must not call
> it from eld_notify.  The eld_notify itself is called in the modeset
> lock, and it leads to a deadlock since get_eld takes the modeset lock,
> too.  You need to call get_eld in a work, for example, in such a case.
> We'll see the actual implementation in the later patch in
> sound/pci/hda/patch_hdmi.c.
> 
> For achieving this implementation, 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.  It's protected with the modeset
> lock as well.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2:
> * Use modeset lock for get_eld lock, drop av mutex
> * Return the expected size from get_eld, not the copied size
> 
>  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  include/drm/i915_component.h       |  6 ++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 0c38cc6c82ae..1965a61769ea 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;
> +
> +	drm_modeset_lock_all(drm_dev);

This is super expensive and shouldn't ever be used in new code. So either
just the connection_mutex or resurrect the av_mutex and just cache what
you need under that. Tbh I prefer the separate lock + cache for such
specific things since it completely avoids spreading and entangling
locking contexts. We use the same design to get modeset information into
the PSR tracking, FBC tracking and other code which sits between KMS and
other subsystems.

> +	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 = drm_eld_size(eld);
> +			memcpy(buf, eld, min(max_bytes, ret));
> +			break;
> +		}
> +	}
> +
> +	drm_modeset_unlock_all(drm_dev);
> +	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..013779db7d24 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 {
> @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
>  	 * pin sense and/or ELD information has changed.
>  	 * @audio_ptr:		HDA driver object
>  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> +	 *
> +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> +	 * from the notifier callback as it may lead to deadlocks.

With av_mutex we don't even need that note here ;-)
-Daniel

>  	 */
>  	void (*pin_eld_notify)(void *audio_ptr, int port);
>  };
> -- 
> 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 v2 1/9] drm/i915: Remove superfluous NULL check

[Daniel Vetter]

On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> to_intel_crtc() always returns a non-NULL pointer.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Queued for -next, thanks for the patch.
-Daniel

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

-- 
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 v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 11:21:02 +0100,
Daniel Vetter wrote:
> 
> On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > the audio driver to fetch the current audio status and ELD of the
> > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > valid, zero if no valid ELD is found, or a negative error code.  The
> > current state of audio on/off is stored in the given pointer, too.
> > 
> > Note that the returned size isn't limited to the given max bytes.  If
> > the size is greater than the max bytes, it means that only a part of
> > ELD has been copied back.
> > 
> > A big warning about the usage of this callback is: you must not call
> > it from eld_notify.  The eld_notify itself is called in the modeset
> > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > too.  You need to call get_eld in a work, for example, in such a case.
> > We'll see the actual implementation in the later patch in
> > sound/pci/hda/patch_hdmi.c.
> > 
> > For achieving this implementation, 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.  It's protected with the modeset
> > lock as well.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2:
> > * Use modeset lock for get_eld lock, drop av mutex
> > * Return the expected size from get_eld, not the copied size
> > 
> >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  include/drm/i915_component.h       |  6 ++++++
> >  3 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 0c38cc6c82ae..1965a61769ea 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;
> > +
> > +	drm_modeset_lock_all(drm_dev);
> 
> This is super expensive and shouldn't ever be used in new code. So either
> just the connection_mutex or resurrect the av_mutex and just cache what
> you need under that.

OK, I need to make it harder, then.

> Tbh I prefer the separate lock + cache for such
> specific things since it completely avoids spreading and entangling
> locking contexts. We use the same design to get modeset information into
> the PSR tracking, FBC tracking and other code which sits between KMS and
> other subsystems.

I didn't want to be involved with the modeset lock, but it has to be.
This function calls drm_select_eld() and it requires both
mode_config.mutex and connection_mutex.

(snip)
> >  struct i915_audio_component_audio_ops {
> > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> >  	 * pin sense and/or ELD information has changed.
> >  	 * @audio_ptr:		HDA driver object
> >  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> > +	 *
> > +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> > +	 * from the notifier callback as it may lead to deadlocks.
> 
> With av_mutex we don't even need that note here ;-)

So here is the problem.  av_mutex itself doesn't suffice for
drm_select_eld(), and taking the modeset lock leads to a deadlock when
invoked from eld_notify.

Maybe one alternative is to pass the audio state and ELD bytes already
in eld_notify itself.  Then it doesn't have to call get_eld from
there.  But we still need the explicit fetch in some cases (at the
first probe and at resume), so get_eld op is still required.  Then it
needs to take locks by itself.


thanks,

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

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

[Ville Syrjälä]

On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 11:21:02 +0100,
> Daniel Vetter wrote:
> > 
> > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > the audio driver to fetch the current audio status and ELD of the
> > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > current state of audio on/off is stored in the given pointer, too.
> > > 
> > > Note that the returned size isn't limited to the given max bytes.  If
> > > the size is greater than the max bytes, it means that only a part of
> > > ELD has been copied back.
> > > 
> > > A big warning about the usage of this callback is: you must not call
> > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > too.  You need to call get_eld in a work, for example, in such a case.
> > > We'll see the actual implementation in the later patch in
> > > sound/pci/hda/patch_hdmi.c.
> > > 
> > > For achieving this implementation, 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.  It's protected with the modeset
> > > lock as well.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2:
> > > * Use modeset lock for get_eld lock, drop av mutex
> > > * Return the expected size from get_eld, not the copied size
> > > 
> > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > >  include/drm/i915_component.h       |  6 ++++++
> > >  3 files changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > index 0c38cc6c82ae..1965a61769ea 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;
> > > +
> > > +	drm_modeset_lock_all(drm_dev);
> > 
> > This is super expensive and shouldn't ever be used in new code. So either
> > just the connection_mutex or resurrect the av_mutex and just cache what
> > you need under that.
> 
> OK, I need to make it harder, then.
> 
> > Tbh I prefer the separate lock + cache for such
> > specific things since it completely avoids spreading and entangling
> > locking contexts. We use the same design to get modeset information into
> > the PSR tracking, FBC tracking and other code which sits between KMS and
> > other subsystems.
> 
> I didn't want to be involved with the modeset lock, but it has to be.
> This function calls drm_select_eld() and it requires both
> mode_config.mutex and connection_mutex.

drm_select_eld() would seem pointless to me if we cached the
required information somewhere. But we'd still need to actually
get the eld, and that means either caching it again somewhere,
or perhaps it would be better to move the drm_edid_to_eld() call to
happen at modeset audio enable time protected by the av_mutex?
That way connector->eld contents would remain constant as long
as we have a mode set.

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

Re: [PATCH v2 1/9] drm/i915: Remove superfluous NULL check

[Ville Syrjälä]

On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> to_intel_crtc() always returns a non-NULL pointer.

Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
breakage on our hands. Or maybe the atomic work has gotten rid of that
assumption, but at least we used to depend on that heavily.

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

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

Re: [Intel-gfx] [PATCH v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 13:10:01 +0100,
Ville Syrjälä wrote:
> 
> On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 11:21:02 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > > the audio driver to fetch the current audio status and ELD of the
> > > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > > current state of audio on/off is stored in the given pointer, too.
> > > > 
> > > > Note that the returned size isn't limited to the given max bytes.  If
> > > > the size is greater than the max bytes, it means that only a part of
> > > > ELD has been copied back.
> > > > 
> > > > A big warning about the usage of this callback is: you must not call
> > > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > > too.  You need to call get_eld in a work, for example, in such a case.
> > > > We'll see the actual implementation in the later patch in
> > > > sound/pci/hda/patch_hdmi.c.
> > > > 
> > > > For achieving this implementation, 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.  It's protected with the modeset
> > > > lock as well.
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > > v1->v2:
> > > > * Use modeset lock for get_eld lock, drop av mutex
> > > > * Return the expected size from get_eld, not the copied size
> > > > 
> > > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > > >  include/drm/i915_component.h       |  6 ++++++
> > > >  3 files changed, 47 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 0c38cc6c82ae..1965a61769ea 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;
> > > > +
> > > > +	drm_modeset_lock_all(drm_dev);
> > > 
> > > This is super expensive and shouldn't ever be used in new code. So either
> > > just the connection_mutex or resurrect the av_mutex and just cache what
> > > you need under that.
> > 
> > OK, I need to make it harder, then.
> > 
> > > Tbh I prefer the separate lock + cache for such
> > > specific things since it completely avoids spreading and entangling
> > > locking contexts. We use the same design to get modeset information into
> > > the PSR tracking, FBC tracking and other code which sits between KMS and
> > > other subsystems.
> > 
> > I didn't want to be involved with the modeset lock, but it has to be.
> > This function calls drm_select_eld() and it requires both
> > mode_config.mutex and connection_mutex.
> 
> drm_select_eld() would seem pointless to me if we cached the
> required information somewhere. But we'd still need to actually
> get the eld, and that means either caching it again somewhere,
> or perhaps it would be better to move the drm_edid_to_eld() call to
> happen at modeset audio enable time protected by the av_mutex?
> That way connector->eld contents would remain constant as long
> as we have a mode set.

Yeah, this is another idea that came to my mind during lunch, too, and
already started coding it ;)


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 1/9] drm/i915: Remove superfluous NULL check

[Takashi Iwai]

On Fri, 04 Dec 2015 13:16:46 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > to_intel_crtc() always returns a non-NULL pointer.
> 
> Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> breakage on our hands. Or maybe the atomic work has gotten rid of that
> assumption, but at least we used to depend on that heavily.

Well, to_intel_crtc() has been always container_of() since the very
beginning of universe:

commit 79e539453b34e35f39299a899d263b0a1f1670bd
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Nov 7 14:24:08 2008 -0800

    DRM: i915: add mode setting support
    
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_drv.h
....
+#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
....


Takashi

> 
> > 
> > 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
> 
> -- 
> Ville Syrjälä
> Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/9] drm/i915: Remove superfluous NULL check

[Ville Syrjälä]

On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 13:16:46 +0100,
> Ville Syrjälä wrote:
> > 
> > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > to_intel_crtc() always returns a non-NULL pointer.
> > 
> > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > assumption, but at least we used to depend on that heavily.
> 
> Well, to_intel_crtc() has been always container_of() since the very
> beginning of universe:
> 
> commit 79e539453b34e35f39299a899d263b0a1f1670bd
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Fri Nov 7 14:24:08 2008 -0800
> 
>     DRM: i915: add mode setting support
>     
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> ....
> +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> ....

Yes, but 
struct intel_crtc {
	struct drm_crtc base;
	...
}

So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.

I once suggested that someone should add a compile time assert to
make sure this always holds for us, but no one took the bait.

> 
> 
> Takashi
> 
> > 
> > > 
> > > 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > 

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/9] drm/i915: Remove superfluous NULL check

[Takashi Iwai]

On Fri, 04 Dec 2015 14:07:03 +0100,
Ville Syrjälä wrote:
> 
> On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 13:16:46 +0100,
> > Ville Syrjälä wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > > to_intel_crtc() always returns a non-NULL pointer.
> > > 
> > > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > > assumption, but at least we used to depend on that heavily.
> > 
> > Well, to_intel_crtc() has been always container_of() since the very
> > beginning of universe:
> > 
> > commit 79e539453b34e35f39299a899d263b0a1f1670bd
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Fri Nov 7 14:24:08 2008 -0800
> > 
> >     DRM: i915: add mode setting support
> >     
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > ....
> > +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> > ....
> 
> Yes, but 
> struct intel_crtc {
> 	struct drm_crtc base;
> 	...
> }
> 
> So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.
> 
> I once suggested that someone should add a compile time assert to
> make sure this always holds for us, but no one took the bait.

Argh, only from the usage of container_of(), no one expects this
implicit assumption :-<  Indeed intel code has lots of these silent
rules, e.g. calling kfree() for the base class object.

Daniel, could you please take my patch back?


thanks,

Takashi

> 
> > 
> > 
> > Takashi
> > 
> > > 
> > > > 
> > > > 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
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/9] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

On Tue, 01 Dec 2015 17:09:53 +0100,
Takashi Iwai wrote:
> 
> This patch adds a reverse mapping from a digital port number to
> intel_encoder object containing the corresponding intel_digital_port.
> It simplifies the query of the encoder a lot.

While this is good for a code reduction, I guess it's better to leave
away for now, as there will be more changes there for MST support.
It may put yet another loop, and the mapping implemented here might
not be the best way.  So I'm going to drop this from the next
patchset.

If anyone still thinks this is worth to include, let me know.  I'll
re-add this.


thanks,

Takashi

> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_audio.c | 22 ++--------------------
>  drivers/gpu/drm/i915/intel_ddi.c   |  1 +
>  drivers/gpu/drm/i915/intel_dp.c    |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c  |  2 ++
>  5 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 95bb27de774f..3483d8125eac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1955,6 +1955,8 @@ struct drm_i915_private {
>  	/* perform PHY state sanity checks? */
>  	bool chv_phy_assert[2];
>  
> +	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 19958bdb9bd0..67ee99f00ddd 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -630,28 +630,10 @@ 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_crtc *crtc;
>  	struct drm_display_mode *mode;
> @@ -668,7 +650,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  
>  	mutex_lock(&dev_priv->av_mutex);
>  	/* 1. get the pipe */
> -	intel_encoder = audio_port_to_encoder(drm_dev, port);
> +	intel_encoder = dev_priv->dig_port_map[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);
> @@ -725,7 +707,7 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  	int ret = -EINVAL;
>  
>  	drm_modeset_lock_all(drm_dev);
> -	intel_encoder = audio_port_to_encoder(drm_dev, port);
> +	intel_encoder = dev_priv->dig_port_map[port];
>  	if (intel_encoder) {
>  		ret = 0;
>  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a6752a61d99f..6770110a4075 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3285,6 +3285,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->get_config = intel_ddi_get_config;
>  
>  	intel_dig_port->port = port;
> +	dev_priv->dig_port_map[port] = intel_encoder;
>  	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
>  					  (DDI_BUF_PORT_REVERSAL |
>  					   DDI_A_4_LANES);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 09bdd94ca3ba..1c02c6466f30 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6201,6 +6201,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	}
>  
>  	intel_dig_port->port = port;
> +	dev_priv->dig_port_map[port] = intel_encoder;
>  	intel_dig_port->dp.output_reg = output_reg;
>  
>  	intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 9eafa191cee2..1d9f522a6679 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2133,6 +2133,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  
>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_digital_port *intel_dig_port;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_connector *intel_connector;
> @@ -2201,6 +2202,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  		intel_encoder->cloneable |= 1 << INTEL_OUTPUT_HDMI;
>  
>  	intel_dig_port->port = port;
> +	dev_priv->dig_port_map[port] = intel_encoder;
>  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>  	intel_dig_port->dp.output_reg = 0;
>  
> -- 
> 2.6.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/9] drm/i915: Remove superfluous NULL check

[Daniel Vetter]

On Fri, Dec 04, 2015 at 02:12:14PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 14:07:03 +0100,
> Ville Syrjälä wrote:
> > 
> > On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> > > On Fri, 04 Dec 2015 13:16:46 +0100,
> > > Ville Syrjälä wrote:
> > > > 
> > > > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > > > to_intel_crtc() always returns a non-NULL pointer.
> > > > 
> > > > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > > > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > > > assumption, but at least we used to depend on that heavily.
> > > 
> > > Well, to_intel_crtc() has been always container_of() since the very
> > > beginning of universe:
> > > 
> > > commit 79e539453b34e35f39299a899d263b0a1f1670bd
> > > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Date:   Fri Nov 7 14:24:08 2008 -0800
> > > 
> > >     DRM: i915: add mode setting support
> > >     
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > ....
> > > +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> > > ....
> > 
> > Yes, but 
> > struct intel_crtc {
> > 	struct drm_crtc base;
> > 	...
> > }
> > 
> > So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.
> > 
> > I once suggested that someone should add a compile time assert to
> > make sure this always holds for us, but no one took the bait.
> 
> Argh, only from the usage of container_of(), no one expects this
> implicit assumption :-<  Indeed intel code has lots of these silent
> rules, e.g. calling kfree() for the base class object.
> 
> Daniel, could you please take my patch back?

Oops. Patch reverted, sorry for the mess.
-Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > > 
> > > 
> > > Takashi
> > > 
> > > > 
> > > > > 
> > > > > 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
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > 
> _______________________________________________
> 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 v2 2/9] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 11:21:02 +0100,
> Daniel Vetter wrote:
> > 
> > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > the audio driver to fetch the current audio status and ELD of the
> > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > current state of audio on/off is stored in the given pointer, too.
> > > 
> > > Note that the returned size isn't limited to the given max bytes.  If
> > > the size is greater than the max bytes, it means that only a part of
> > > ELD has been copied back.
> > > 
> > > A big warning about the usage of this callback is: you must not call
> > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > too.  You need to call get_eld in a work, for example, in such a case.
> > > We'll see the actual implementation in the later patch in
> > > sound/pci/hda/patch_hdmi.c.
> > > 
> > > For achieving this implementation, 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.  It's protected with the modeset
> > > lock as well.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2:
> > > * Use modeset lock for get_eld lock, drop av mutex
> > > * Return the expected size from get_eld, not the copied size
> > > 
> > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > >  include/drm/i915_component.h       |  6 ++++++
> > >  3 files changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > index 0c38cc6c82ae..1965a61769ea 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;
> > > +
> > > +	drm_modeset_lock_all(drm_dev);
> > 
> > This is super expensive and shouldn't ever be used in new code. So either
> > just the connection_mutex or resurrect the av_mutex and just cache what
> > you need under that.
> 
> OK, I need to make it harder, then.
> 
> > Tbh I prefer the separate lock + cache for such
> > specific things since it completely avoids spreading and entangling
> > locking contexts. We use the same design to get modeset information into
> > the PSR tracking, FBC tracking and other code which sits between KMS and
> > other subsystems.
> 
> I didn't want to be involved with the modeset lock, but it has to be.
> This function calls drm_select_eld() and it requires both
> mode_config.mutex and connection_mutex.
> 
> (snip)
> > >  struct i915_audio_component_audio_ops {
> > > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> > >  	 * pin sense and/or ELD information has changed.
> > >  	 * @audio_ptr:		HDA driver object
> > >  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> > > +	 *
> > > +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> > > +	 * from the notifier callback as it may lead to deadlocks.
> > 
> > With av_mutex we don't even need that note here ;-)
> 
> So here is the problem.  av_mutex itself doesn't suffice for
> drm_select_eld(), and taking the modeset lock leads to a deadlock when
> invoked from eld_notify.

Yeah, select_eld is broken currently in atomic land, and we need to fix
that. It's by far not the only one that's iffy.

> Maybe one alternative is to pass the audio state and ELD bytes already
> in eld_notify itself.  Then it doesn't have to call get_eld from
> there.  But we still need the explicit fetch in some cases (at the
> first probe and at resume), so get_eld op is still required.  Then it
> needs to take locks by itself.

Well my idea was that in the enable/disable hooks (where we should hold
relevant modeset locks already, except for that icky unsolved thing I need
to take care of anyway), and store a copy (protected by av_lock). Then
get_eld would only look at that copy. That kind of "cache relevant data,
protected with new leaf lock" trick is what I meant we should use here,
and it's the usual approach to avoid acquiring modeset locks from random
other subsystems (since that ends in deadlocks sooner or later). So no
calling drm_get_eld from the new get_eld hook at all.

There's still the problem that currently calling drm_get_eld is broken
with atomic modesets even in i915 audio enable/disable functions. But
that's a preexisting problem with atomic, and one I know we need to fix
still before we can enable atomic for real (all legacy paths get away
since there we take more locks).

Cheers, 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 v2 4/9] drm/i915: Add reverse mapping between port and intel_encoder

[Daniel Vetter]

On Fri, Dec 04, 2015 at 03:40:03PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 17:09:53 +0100,
> Takashi Iwai wrote:
> > 
> > This patch adds a reverse mapping from a digital port number to
> > intel_encoder object containing the corresponding intel_digital_port.
> > It simplifies the query of the encoder a lot.
> 
> While this is good for a code reduction, I guess it's better to leave
> away for now, as there will be more changes there for MST support.
> It may put yet another loop, and the mapping implemented here might
> not be the best way.  So I'm going to drop this from the next
> patchset.
> 
> If anyone still thinks this is worth to include, let me know.  I'll
> re-add this.

Yeah I think this looks good. Fixing up conflicts with MST shouldn't be
that much trouble really.
-Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 22 ++--------------------
> >  drivers/gpu/drm/i915/intel_ddi.c   |  1 +
> >  drivers/gpu/drm/i915/intel_dp.c    |  1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c  |  2 ++
> >  5 files changed, 8 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 95bb27de774f..3483d8125eac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1955,6 +1955,8 @@ struct drm_i915_private {
> >  	/* perform PHY state sanity checks? */
> >  	bool chv_phy_assert[2];
> >  
> > +	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> > +
> >  	/*
> >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> >  	 * will be rejected. Instead look for a better place.
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 19958bdb9bd0..67ee99f00ddd 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -630,28 +630,10 @@ 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_crtc *crtc;
> >  	struct drm_display_mode *mode;
> > @@ -668,7 +650,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> >  
> >  	mutex_lock(&dev_priv->av_mutex);
> >  	/* 1. get the pipe */
> > -	intel_encoder = audio_port_to_encoder(drm_dev, port);
> > +	intel_encoder = dev_priv->dig_port_map[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);
> > @@ -725,7 +707,7 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> >  	int ret = -EINVAL;
> >  
> >  	drm_modeset_lock_all(drm_dev);
> > -	intel_encoder = audio_port_to_encoder(drm_dev, port);
> > +	intel_encoder = dev_priv->dig_port_map[port];
> >  	if (intel_encoder) {
> >  		ret = 0;
> >  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index a6752a61d99f..6770110a4075 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3285,6 +3285,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
> >  	intel_encoder->get_config = intel_ddi_get_config;
> >  
> >  	intel_dig_port->port = port;
> > +	dev_priv->dig_port_map[port] = intel_encoder;
> >  	intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) &
> >  					  (DDI_BUF_PORT_REVERSAL |
> >  					   DDI_A_4_LANES);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 09bdd94ca3ba..1c02c6466f30 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6201,6 +6201,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >  	}
> >  
> >  	intel_dig_port->port = port;
> > +	dev_priv->dig_port_map[port] = intel_encoder;
> >  	intel_dig_port->dp.output_reg = output_reg;
> >  
> >  	intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 9eafa191cee2..1d9f522a6679 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2133,6 +2133,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  
> >  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_digital_port *intel_dig_port;
> >  	struct intel_encoder *intel_encoder;
> >  	struct intel_connector *intel_connector;
> > @@ -2201,6 +2202,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> >  		intel_encoder->cloneable |= 1 << INTEL_OUTPUT_HDMI;
> >  
> >  	intel_dig_port->port = port;
> > +	dev_priv->dig_port_map[port] = intel_encoder;
> >  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
> >  	intel_dig_port->dp.output_reg = 0;
> >  
> > -- 
> > 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 v2 4/9] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

On Fri, 04 Dec 2015 16:02:29 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 03:40:03PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 17:09:53 +0100,
> > Takashi Iwai wrote:
> > > 
> > > This patch adds a reverse mapping from a digital port number to
> > > intel_encoder object containing the corresponding intel_digital_port.
> > > It simplifies the query of the encoder a lot.
> > 
> > While this is good for a code reduction, I guess it's better to leave
> > away for now, as there will be more changes there for MST support.
> > It may put yet another loop, and the mapping implemented here might
> > not be the best way.  So I'm going to drop this from the next
> > patchset.
> > 
> > If anyone still thinks this is worth to include, let me know.  I'll
> > re-add this.
> 
> Yeah I think this looks good. Fixing up conflicts with MST shouldn't be
> that much trouble really.

Alright, resurrected from ash.


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

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

[Takashi Iwai]

On Fri, 04 Dec 2015 16:00:46 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 11:21:02 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > > the audio driver to fetch the current audio status and ELD of the
> > > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > > current state of audio on/off is stored in the given pointer, too.
> > > > 
> > > > Note that the returned size isn't limited to the given max bytes.  If
> > > > the size is greater than the max bytes, it means that only a part of
> > > > ELD has been copied back.
> > > > 
> > > > A big warning about the usage of this callback is: you must not call
> > > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > > too.  You need to call get_eld in a work, for example, in such a case.
> > > > We'll see the actual implementation in the later patch in
> > > > sound/pci/hda/patch_hdmi.c.
> > > > 
> > > > For achieving this implementation, 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.  It's protected with the modeset
> > > > lock as well.
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > > v1->v2:
> > > > * Use modeset lock for get_eld lock, drop av mutex
> > > > * Return the expected size from get_eld, not the copied size
> > > > 
> > > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > > >  include/drm/i915_component.h       |  6 ++++++
> > > >  3 files changed, 47 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 0c38cc6c82ae..1965a61769ea 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;
> > > > +
> > > > +	drm_modeset_lock_all(drm_dev);
> > > 
> > > This is super expensive and shouldn't ever be used in new code. So either
> > > just the connection_mutex or resurrect the av_mutex and just cache what
> > > you need under that.
> > 
> > OK, I need to make it harder, then.
> > 
> > > Tbh I prefer the separate lock + cache for such
> > > specific things since it completely avoids spreading and entangling
> > > locking contexts. We use the same design to get modeset information into
> > > the PSR tracking, FBC tracking and other code which sits between KMS and
> > > other subsystems.
> > 
> > I didn't want to be involved with the modeset lock, but it has to be.
> > This function calls drm_select_eld() and it requires both
> > mode_config.mutex and connection_mutex.
> > 
> > (snip)
> > > >  struct i915_audio_component_audio_ops {
> > > > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> > > >  	 * pin sense and/or ELD information has changed.
> > > >  	 * @audio_ptr:		HDA driver object
> > > >  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> > > > +	 *
> > > > +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> > > > +	 * from the notifier callback as it may lead to deadlocks.
> > > 
> > > With av_mutex we don't even need that note here ;-)
> > 
> > So here is the problem.  av_mutex itself doesn't suffice for
> > drm_select_eld(), and taking the modeset lock leads to a deadlock when
> > invoked from eld_notify.
> 
> Yeah, select_eld is broken currently in atomic land, and we need to fix
> that. It's by far not the only one that's iffy.
> 
> > Maybe one alternative is to pass the audio state and ELD bytes already
> > in eld_notify itself.  Then it doesn't have to call get_eld from
> > there.  But we still need the explicit fetch in some cases (at the
> > first probe and at resume), so get_eld op is still required.  Then it
> > needs to take locks by itself.
> 
> Well my idea was that in the enable/disable hooks (where we should hold
> relevant modeset locks already, except for that icky unsolved thing I need
> to take care of anyway), and store a copy (protected by av_lock). Then
> get_eld would only look at that copy. That kind of "cache relevant data,
> protected with new leaf lock" trick is what I meant we should use here,
> and it's the usual approach to avoid acquiring modeset locks from random
> other subsystems (since that ends in deadlocks sooner or later). So no
> calling drm_get_eld from the new get_eld hook at all.
> 
> There's still the problem that currently calling drm_get_eld is broken
> with atomic modesets even in i915 audio enable/disable functions. But
> that's a preexisting problem with atomic, and one I know we need to fix
> still before we can enable atomic for real (all legacy paths get away
> since there we take more locks).

While I'm coding it, I wonder whether we really need to cache/copy the
whole ELD content again.  Wouldn't tracking the drm_connector point
work?  If the connector might be removed / updated, then it involves
with the modeset updates, and calling audio_codec_disable() in
anyway.

FWIW, the below is the draft version I finished rewriting now
(just compile-tested).


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/i915: Add get_eld audio component

Implement a new i915_audio_component_ops, get_eld().  It's called by
the audio driver to fetch the current audio status and ELD of the
given HDMI/DP port.  It returns the size of expected ELD bytes if it's
valid, zero if no valid ELD is found, or a negative error code.  The
current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes.  If
the size is greater than the max bytes, it means that only a part of
ELD has been copied back.

For achieving this implementation, a new field audio_connector is
added to struct intel_digital_port.  It points to the connector
assigned to the given digital port.  It's set/reset at each audio
enable/disable call in intel_audio.c, and protected with av_mutex.

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

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 4dccd9b003a1..7f45e73f58f5 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -525,6 +525,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 		dev_priv->display.audio_codec_enable(connector, intel_encoder,
 						     adjusted_mode);
 
+	mutex_lock(&dev_priv->av_mutex);
+	intel_dig_port->audio_connector = connector;
+	mutex_unlock(&dev_priv->av_mutex);
+
 	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
@@ -548,6 +552,10 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	if (dev_priv->display.audio_codec_disable)
 		dev_priv->display.audio_codec_disable(intel_encoder);
 
+	mutex_lock(&dev_priv->av_mutex);
+	intel_dig_port->audio_connector = NULL;
+	mutex_unlock(&dev_priv->av_mutex);
+
 	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
@@ -706,6 +714,39 @@ 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;
+	const u8 *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_connector != NULL;
+			if (!*enabled)
+				break;
+			eld = intel_dig_port->audio_connector->eld;
+			ret = drm_eld_size(eld);
+			memcpy(buf, eld, min(max_bytes, 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 +754,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..607922e4937c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -800,6 +800,8 @@ struct intel_digital_port {
 	struct intel_hdmi hdmi;
 	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
 	bool release_cl2_override;
+	/* for communication with audio component; protected by av_mutex */
+	const struct drm_connector *audio_connector;
 };
 
 struct intel_dp_mst_encoder {
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

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

[Daniel Vetter]

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 16:00:46 +0100,
> Daniel Vetter wrote:
> > 
> > On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> > > On Fri, 04 Dec 2015 11:21:02 +0100,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > > > the audio driver to fetch the current audio status and ELD of the
> > > > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > > > current state of audio on/off is stored in the given pointer, too.
> > > > > 
> > > > > Note that the returned size isn't limited to the given max bytes.  If
> > > > > the size is greater than the max bytes, it means that only a part of
> > > > > ELD has been copied back.
> > > > > 
> > > > > A big warning about the usage of this callback is: you must not call
> > > > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > > > too.  You need to call get_eld in a work, for example, in such a case.
> > > > > We'll see the actual implementation in the later patch in
> > > > > sound/pci/hda/patch_hdmi.c.
> > > > > 
> > > > > For achieving this implementation, 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.  It's protected with the modeset
> > > > > lock as well.
> > > > > 
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > ---
> > > > > v1->v2:
> > > > > * Use modeset lock for get_eld lock, drop av mutex
> > > > > * Return the expected size from get_eld, not the copied size
> > > > > 
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > > > >  include/drm/i915_component.h       |  6 ++++++
> > > > >  3 files changed, 47 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 0c38cc6c82ae..1965a61769ea 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;
> > > > > +
> > > > > +	drm_modeset_lock_all(drm_dev);
> > > > 
> > > > This is super expensive and shouldn't ever be used in new code. So either
> > > > just the connection_mutex or resurrect the av_mutex and just cache what
> > > > you need under that.
> > > 
> > > OK, I need to make it harder, then.
> > > 
> > > > Tbh I prefer the separate lock + cache for such
> > > > specific things since it completely avoids spreading and entangling
> > > > locking contexts. We use the same design to get modeset information into
> > > > the PSR tracking, FBC tracking and other code which sits between KMS and
> > > > other subsystems.
> > > 
> > > I didn't want to be involved with the modeset lock, but it has to be.
> > > This function calls drm_select_eld() and it requires both
> > > mode_config.mutex and connection_mutex.
> > > 
> > > (snip)
> > > > >  struct i915_audio_component_audio_ops {
> > > > > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> > > > >  	 * pin sense and/or ELD information has changed.
> > > > >  	 * @audio_ptr:		HDA driver object
> > > > >  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> > > > > +	 *
> > > > > +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> > > > > +	 * from the notifier callback as it may lead to deadlocks.
> > > > 
> > > > With av_mutex we don't even need that note here ;-)
> > > 
> > > So here is the problem.  av_mutex itself doesn't suffice for
> > > drm_select_eld(), and taking the modeset lock leads to a deadlock when
> > > invoked from eld_notify.
> > 
> > Yeah, select_eld is broken currently in atomic land, and we need to fix
> > that. It's by far not the only one that's iffy.
> > 
> > > Maybe one alternative is to pass the audio state and ELD bytes already
> > > in eld_notify itself.  Then it doesn't have to call get_eld from
> > > there.  But we still need the explicit fetch in some cases (at the
> > > first probe and at resume), so get_eld op is still required.  Then it
> > > needs to take locks by itself.
> > 
> > Well my idea was that in the enable/disable hooks (where we should hold
> > relevant modeset locks already, except for that icky unsolved thing I need
> > to take care of anyway), and store a copy (protected by av_lock). Then
> > get_eld would only look at that copy. That kind of "cache relevant data,
> > protected with new leaf lock" trick is what I meant we should use here,
> > and it's the usual approach to avoid acquiring modeset locks from random
> > other subsystems (since that ends in deadlocks sooner or later). So no
> > calling drm_get_eld from the new get_eld hook at all.
> > 
> > There's still the problem that currently calling drm_get_eld is broken
> > with atomic modesets even in i915 audio enable/disable functions. But
> > that's a preexisting problem with atomic, and one I know we need to fix
> > still before we can enable atomic for real (all legacy paths get away
> > since there we take more locks).
> 
> While I'm coding it, I wonder whether we really need to cache/copy the
> whole ELD content again.  Wouldn't tracking the drm_connector point
> work?  If the connector might be removed / updated, then it involves
> with the modeset updates, and calling audio_codec_disable() in
> anyway.

So for context with atomic the locking completely splits the display
configuration from output detection. Display config is protected by a pile
of wait/wound mutexes (abstracted a bit with drm_modeset_lock.c), output
probing is protected by dev->mode_config.mutex.

Now in the compute config phase we need to inspect probe state to figure
out what we should do (stuff like enabling audio), but after that the
configuration should stay static until userspace asks for an update.
Otherwise it will just end up in confusion.

My idea to fix this all is to track all this stuff (so including has_audio
and the eld and whatever else we need) in the atomic state structures. And
set up a bunch of _get functions (for use in compute config hooks) and
_set functions (for updating probed state) with internal locking. We
really need to do this copying, otherwise we'll always run int fun stuff
with the configuration changing underneath us, or just leaking of locks
from one side to the other, rendering the fine-grained locking a bit
pointless.

So in the end there'll be 2 copies: probe -> modeset code, and the one you
added here which copies modeset code -> audio code. It looks a bit silly,
but imo it's the simplest solution and should be easy to add locking
asserts to _get and _set functions to make sure they're always called in
the right context.

> FWIW, the below is the draft version I finished rewriting now
> (just compile-tested).

One question below.

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] drm/i915: Add get_eld audio component
> 
> Implement a new i915_audio_component_ops, get_eld().  It's called by
> the audio driver to fetch the current audio status and ELD of the
> given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> valid, zero if no valid ELD is found, or a negative error code.  The
> current state of audio on/off is stored in the given pointer, too.
> 
> Note that the returned size isn't limited to the given max bytes.  If
> the size is greater than the max bytes, it means that only a part of
> ELD has been copied back.
> 
> For achieving this implementation, a new field audio_connector is
> added to struct intel_digital_port.  It points to the connector
> assigned to the given digital port.  It's set/reset at each audio
> enable/disable call in intel_audio.c, and protected with av_mutex.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  include/drm/i915_component.h       |  3 +++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 4dccd9b003a1..7f45e73f58f5 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -525,6 +525,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  		dev_priv->display.audio_codec_enable(connector, intel_encoder,
>  						     adjusted_mode);
>  
> +	mutex_lock(&dev_priv->av_mutex);
> +	intel_dig_port->audio_connector = connector;
> +	mutex_unlock(&dev_priv->av_mutex);
> +
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
>  }
> @@ -548,6 +552,10 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	if (dev_priv->display.audio_codec_disable)
>  		dev_priv->display.audio_codec_disable(intel_encoder);
>  
> +	mutex_lock(&dev_priv->av_mutex);
> +	intel_dig_port->audio_connector = NULL;
> +	mutex_unlock(&dev_priv->av_mutex);
> +
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
>  }
> @@ -706,6 +714,39 @@ 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;
> +	const u8 *eld;
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&dev_priv->av_mutex);
> +	for_each_intel_encoder(drm_dev, intel_encoder) {

I guess the loop will dissipate with your cleanup patch to have a audio
prot -> intel_dig_port mapping?

lgtm otherwise.
-Daniel

> +		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_connector != NULL;
> +			if (!*enabled)
> +				break;
> +			eld = intel_dig_port->audio_connector->eld;
> +			ret = drm_eld_size(eld);
> +			memcpy(buf, eld, min(max_bytes, 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 +754,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..607922e4937c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -800,6 +800,8 @@ struct intel_digital_port {
>  	struct intel_hdmi hdmi;
>  	enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
>  	bool release_cl2_override;
> +	/* for communication with audio component; protected by av_mutex */
> +	const struct drm_connector *audio_connector;
>  };
>  
>  struct intel_dp_mst_encoder {
> 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
> 

-- 
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 v2 2/9] drm/i915: Add get_eld audio component

[Daniel Vetter]

On Fri, Dec 04, 2015 at 04:15:24PM +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

One more: You seem to still be on an old baseline, with switched to the
new in-line comment layout. That allows you to spec the callback semantics
in much more detail since it allows real paragraphs.
-Daniel

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

-- 
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 v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 16:53:02 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 16:00:46 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:
> > > > On Fri, 04 Dec 2015 11:21:02 +0100,
> > > > Daniel Vetter wrote:
> > > > > 
> > > > > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:
> > > > > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > > > > the audio driver to fetch the current audio status and ELD of the
> > > > > > given HDMI/DP port.  It returns the size of expected ELD bytes if it's
> > > > > > valid, zero if no valid ELD is found, or a negative error code.  The
> > > > > > current state of audio on/off is stored in the given pointer, too.
> > > > > > 
> > > > > > Note that the returned size isn't limited to the given max bytes.  If
> > > > > > the size is greater than the max bytes, it means that only a part of
> > > > > > ELD has been copied back.
> > > > > > 
> > > > > > A big warning about the usage of this callback is: you must not call
> > > > > > it from eld_notify.  The eld_notify itself is called in the modeset
> > > > > > lock, and it leads to a deadlock since get_eld takes the modeset lock,
> > > > > > too.  You need to call get_eld in a work, for example, in such a case.
> > > > > > We'll see the actual implementation in the later patch in
> > > > > > sound/pci/hda/patch_hdmi.c.
> > > > > > 
> > > > > > For achieving this implementation, 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.  It's protected with the modeset
> > > > > > lock as well.
> > > > > > 
> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > ---
> > > > > > v1->v2:
> > > > > > * Use modeset lock for get_eld lock, drop av mutex
> > > > > > * Return the expected size from get_eld, not the copied size
> > > > > > 
> > > > > >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> > > > > >  include/drm/i915_component.h       |  6 ++++++
> > > > > >  3 files changed, 47 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > index 0c38cc6c82ae..1965a61769ea 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;
> > > > > > +
> > > > > > +	drm_modeset_lock_all(drm_dev);
> > > > > 
> > > > > This is super expensive and shouldn't ever be used in new code. So either
> > > > > just the connection_mutex or resurrect the av_mutex and just cache what
> > > > > you need under that.
> > > > 
> > > > OK, I need to make it harder, then.
> > > > 
> > > > > Tbh I prefer the separate lock + cache for such
> > > > > specific things since it completely avoids spreading and entangling
> > > > > locking contexts. We use the same design to get modeset information into
> > > > > the PSR tracking, FBC tracking and other code which sits between KMS and
> > > > > other subsystems.
> > > > 
> > > > I didn't want to be involved with the modeset lock, but it has to be.
> > > > This function calls drm_select_eld() and it requires both
> > > > mode_config.mutex and connection_mutex.
> > > > 
> > > > (snip)
> > > > > >  struct i915_audio_component_audio_ops {
> > > > > > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> > > > > >  	 * pin sense and/or ELD information has changed.
> > > > > >  	 * @audio_ptr:		HDA driver object
> > > > > >  	 * @port:	Which port has changed (PORTA / PORTB / PORTC etc)
> > > > > > +	 *
> > > > > > +	 * Note that you can't call i915_audio_component_ops.get_eld directly
> > > > > > +	 * from the notifier callback as it may lead to deadlocks.
> > > > > 
> > > > > With av_mutex we don't even need that note here ;-)
> > > > 
> > > > So here is the problem.  av_mutex itself doesn't suffice for
> > > > drm_select_eld(), and taking the modeset lock leads to a deadlock when
> > > > invoked from eld_notify.
> > > 
> > > Yeah, select_eld is broken currently in atomic land, and we need to fix
> > > that. It's by far not the only one that's iffy.
> > > 
> > > > Maybe one alternative is to pass the audio state and ELD bytes already
> > > > in eld_notify itself.  Then it doesn't have to call get_eld from
> > > > there.  But we still need the explicit fetch in some cases (at the
> > > > first probe and at resume), so get_eld op is still required.  Then it
> > > > needs to take locks by itself.
> > > 
> > > Well my idea was that in the enable/disable hooks (where we should hold
> > > relevant modeset locks already, except for that icky unsolved thing I need
> > > to take care of anyway), and store a copy (protected by av_lock). Then
> > > get_eld would only look at that copy. That kind of "cache relevant data,
> > > protected with new leaf lock" trick is what I meant we should use here,
> > > and it's the usual approach to avoid acquiring modeset locks from random
> > > other subsystems (since that ends in deadlocks sooner or later). So no
> > > calling drm_get_eld from the new get_eld hook at all.
> > > 
> > > There's still the problem that currently calling drm_get_eld is broken
> > > with atomic modesets even in i915 audio enable/disable functions. But
> > > that's a preexisting problem with atomic, and one I know we need to fix
> > > still before we can enable atomic for real (all legacy paths get away
> > > since there we take more locks).
> > 
> > While I'm coding it, I wonder whether we really need to cache/copy the
> > whole ELD content again.  Wouldn't tracking the drm_connector point
> > work?  If the connector might be removed / updated, then it involves
> > with the modeset updates, and calling audio_codec_disable() in
> > anyway.
> 
> So for context with atomic the locking completely splits the display
> configuration from output detection. Display config is protected by a pile
> of wait/wound mutexes (abstracted a bit with drm_modeset_lock.c), output
> probing is protected by dev->mode_config.mutex.
> 
> Now in the compute config phase we need to inspect probe state to figure
> out what we should do (stuff like enabling audio), but after that the
> configuration should stay static until userspace asks for an update.
> Otherwise it will just end up in confusion.
 
OK.

> My idea to fix this all is to track all this stuff (so including has_audio
> and the eld and whatever else we need) in the atomic state structures. And
> set up a bunch of _get functions (for use in compute config hooks) and
> _set functions (for updating probed state) with internal locking. We
> really need to do this copying, otherwise we'll always run int fun stuff
> with the configuration changing underneath us, or just leaking of locks
> from one side to the other, rendering the fine-grained locking a bit
> pointless.
> 
> So in the end there'll be 2 copies: probe -> modeset code, and the one you
> added here which copies modeset code -> audio code. It looks a bit silly,
> but imo it's the simplest solution and should be easy to add locking
> asserts to _get and _set functions to make sure they're always called in
> the right context.

Yeah, syncing all these is a really hard job.  Keeping self-contained
is a safer design.


> > FWIW, the below is the draft version I finished rewriting now
> > (just compile-tested).
> 
> One question below.
(snip)
> > +	mutex_lock(&dev_priv->av_mutex);
> > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> 
> I guess the loop will dissipate with your cleanup patch to have a audio
> prot -> intel_dig_port mapping?

Right.  It'll be simplified by the next patch.
I'm going to submit the v3 patchset.

Thanks!


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

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

[Takashi Iwai]

On Fri, 04 Dec 2015 16:54:32 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 04:15:24PM +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
> 
> One more: You seem to still be on an old baseline, with switched to the
> new in-line comment layout. That allows you to spec the callback semantics
> in much more detail since it allows real paragraphs.

Yes, I've been waiting for your (or Dave's) answer to my previous
question: which branch can I use as a solid base?


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

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

[Daniel Vetter]

On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 16:54:32 +0100,
> Daniel Vetter wrote:
> > 
> > On Fri, Dec 04, 2015 at 04:15:24PM +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
> > 
> > One more: You seem to still be on an old baseline, with switched to the
> > new in-line comment layout. That allows you to spec the callback semantics
> > in much more detail since it allows real paragraphs.
> 
> Yes, I've been waiting for your (or Dave's) answer to my previous
> question: which branch can I use as a solid base?

Ooops sorry. drm-next is now open and has everything you need.

git://people.freedesktop.org/~airlied/linux drm-next

Cheers, 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 v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 17:15:59 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 16:54:32 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Dec 04, 2015 at 04:15:24PM +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
> > > 
> > > One more: You seem to still be on an old baseline, with switched to the
> > > new in-line comment layout. That allows you to spec the callback semantics
> > > in much more detail since it allows real paragraphs.
> > 
> > Yes, I've been waiting for your (or Dave's) answer to my previous
> > question: which branch can I use as a solid base?
> 
> Ooops sorry. drm-next is now open and has everything you need.
> 
> git://people.freedesktop.org/~airlied/linux drm-next

Thanks, I'll rebase on it.


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

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

[Takashi Iwai]

On Fri, 04 Dec 2015 17:20:15 +0100,
Takashi Iwai wrote:
> 
> On Fri, 04 Dec 2015 17:15:59 +0100,
> Daniel Vetter wrote:
> > 
> > On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:
> > > On Fri, 04 Dec 2015 16:54:32 +0100,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Fri, Dec 04, 2015 at 04:15:24PM +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
> > > > 
> > > > One more: You seem to still be on an old baseline, with switched to the
> > > > new in-line comment layout. That allows you to spec the callback semantics
> > > > in much more detail since it allows real paragraphs.
> > > 
> > > Yes, I've been waiting for your (or Dave's) answer to my previous
> > > question: which branch can I use as a solid base?
> > 
> > Ooops sorry. drm-next is now open and has everything you need.
> > 
> > git://people.freedesktop.org/~airlied/linux drm-next
> 
> Thanks, I'll rebase on it.

Hmm, this branch gives a compile warning:

drivers/gpu/drm/i915/intel_display.c:5217:0: warning: "for_each_power_domain" redefined
 #define for_each_power_domain(domain, mask)    \
 ^
In file included from drivers/gpu/drm/i915/intel_drv.h:32:0,
                 from drivers/gpu/drm/i915/intel_display.c:36:
drivers/gpu/drm/i915/i915_drv.h:312:0: note: this is the location of the previous definition
 #define for_each_power_domain(domain, mask)    \
 ^
  LD [M]  drivers/gpu/drm/i915/i915.o


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

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

[Daniel Vetter]

On Fri, Dec 04, 2015 at 05:27:12PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 17:20:15 +0100,
> Takashi Iwai wrote:
> > 
> > On Fri, 04 Dec 2015 17:15:59 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:
> > > > On Fri, 04 Dec 2015 16:54:32 +0100,
> > > > Daniel Vetter wrote:
> > > > > 
> > > > > On Fri, Dec 04, 2015 at 04:15:24PM +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
> > > > > 
> > > > > One more: You seem to still be on an old baseline, with switched to the
> > > > > new in-line comment layout. That allows you to spec the callback semantics
> > > > > in much more detail since it allows real paragraphs.
> > > > 
> > > > Yes, I've been waiting for your (or Dave's) answer to my previous
> > > > question: which branch can I use as a solid base?
> > > 
> > > Ooops sorry. drm-next is now open and has everything you need.
> > > 
> > > git://people.freedesktop.org/~airlied/linux drm-next
> > 
> > Thanks, I'll rebase on it.
> 
> Hmm, this branch gives a compile warning:
> 
> drivers/gpu/drm/i915/intel_display.c:5217:0: warning: "for_each_power_domain" redefined
>  #define for_each_power_domain(domain, mask)    \
>  ^
> In file included from drivers/gpu/drm/i915/intel_drv.h:32:0,
>                  from drivers/gpu/drm/i915/intel_display.c:36:
> drivers/gpu/drm/i915/i915_drv.h:312:0: note: this is the location of the previous definition
>  #define for_each_power_domain(domain, mask)    \
>  ^
>   LD [M]  drivers/gpu/drm/i915/i915.o

Hilarious merge fail on my side, the patch to fix it up is queued in
drm-intel.git. I'll send a pull request for that to Dave end of next week
or so.
-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 v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 17:49:43 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 05:27:12PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 17:20:15 +0100,
> > Takashi Iwai wrote:
> > > 
> > > On Fri, 04 Dec 2015 17:15:59 +0100,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:
> > > > > On Fri, 04 Dec 2015 16:54:32 +0100,
> > > > > Daniel Vetter wrote:
> > > > > > 
> > > > > > On Fri, Dec 04, 2015 at 04:15:24PM +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
> > > > > > 
> > > > > > One more: You seem to still be on an old baseline, with switched to the
> > > > > > new in-line comment layout. That allows you to spec the callback semantics
> > > > > > in much more detail since it allows real paragraphs.
> > > > > 
> > > > > Yes, I've been waiting for your (or Dave's) answer to my previous
> > > > > question: which branch can I use as a solid base?
> > > > 
> > > > Ooops sorry. drm-next is now open and has everything you need.
> > > > 
> > > > git://people.freedesktop.org/~airlied/linux drm-next
> > > 
> > > Thanks, I'll rebase on it.
> > 
> > Hmm, this branch gives a compile warning:
> > 
> > drivers/gpu/drm/i915/intel_display.c:5217:0: warning: "for_each_power_domain" redefined
> >  #define for_each_power_domain(domain, mask)    \
> >  ^
> > In file included from drivers/gpu/drm/i915/intel_drv.h:32:0,
> >                  from drivers/gpu/drm/i915/intel_display.c:36:
> > drivers/gpu/drm/i915/i915_drv.h:312:0: note: this is the location of the previous definition
> >  #define for_each_power_domain(domain, mask)    \
> >  ^
> >   LD [M]  drivers/gpu/drm/i915/i915.o
> 
> Hilarious merge fail on my side, the patch to fix it up is queued in
> drm-intel.git. I'll send a pull request for that to Dave end of next week
> or so.

Alright.  Meanwhile I rebased the patchset, and it's enough for
review, at least.  I can rebase again at the final merge time to the
fixed branch.


thanks,

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