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

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

[Takashi Iwai]

Hi,

this is the third revision of the 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.

A few patches have been dropped from this patchset as they were
already applied individually beforehand.  Also, two cleanup patches
for i915 were squashed into one patch.

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


thanks,

Takashi

===

v2->v3:
* Track drm_connector for easier ELD retrieval, remove superfluous
  audio_enabled flag instead
* Back to av_mutex
* Adapt the earlier applied preliminary patches
* Direct invocation of get_eld now instead of deferred call, as we
  have no longer deadlocks
* Rebase to drm-next, update get_eld documentation for new kernel doc

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 (4):
  drm/i915: Add get_eld audio component
  drm/i915: Add reverse mapping between port and intel_encoder
  ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling
  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   |   2 +
 drivers/gpu/drm/i915/intel_hdmi.c  |   2 +
 include/drm/i915_component.h       |  14 +++++
 include/sound/hda_i915.h           |  14 +++++
 sound/hda/hdac_i915.c              |  66 +++++++++++++++++++++++
 sound/pci/hda/patch_hdmi.c         | 104 ++++++++++++++++++++++++++++++-------
 10 files changed, 229 insertions(+), 36 deletions(-)

-- 
2.6.3

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

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

[Takashi Iwai]

Implement a new i915_audio_component_ops, get_eld().  It's called by
the audio driver to fetch the current 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>
---
v2->v3:
* Track drm_connector for easier ELD retrieval, remove superfluous
  audio_enabled flag instead
* Back to av_mutex
* Rebase to drm-next, update get_eld documentation for new kernel doc

 drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 include/drm/i915_component.h       | 14 +++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 9aa83e71b792..eeac9f763110 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -521,6 +521,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);
 }
@@ -544,6 +548,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);
 }
@@ -703,6 +711,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,
@@ -710,6 +751,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = {
 	.codec_wake_override = i915_audio_component_codec_wake_override,
 	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
 	.sync_audio_rate = i915_audio_component_sync_audio_rate,
+	.get_eld	= i915_audio_component_get_eld,
 };
 
 static int i915_audio_component_bind(struct device *i915_dev,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab5c147fa9e9..fe58a5722b16 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -814,6 +814,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 fab13851f95a..b46fa0ef3005 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -65,6 +65,20 @@ struct i915_audio_component_ops {
 	 * sample rate, it will call this function to set n/cts
 	 */
 	int (*sync_audio_rate)(struct device *, int port, int rate);
+	/**
+	 * @get_eld: fill the audio state and ELD bytes for the given port
+	 *
+	 * Called from audio driver to get the HDMI/DP audio state of the given
+	 * digital port, and also fetch ELD bytes to the given pointer.
+	 *
+	 * It returns the byte size of the original ELD (not the actually
+	 * copied size), zero for an invalid ELD, or a negative error code.
+	 *
+	 * Note that the returned size may be over @max_bytes.  Then it
+	 * implies that only a part of ELD has been copied to the buffer.
+	 */
+	int (*get_eld)(struct device *, int port, bool *enabled,
+		       unsigned char *buf, int max_bytes);
 };
 
 /**
-- 
2.6.3

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

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

[Daniel Vetter]

On Fri, Dec 04, 2015 at 06:12:46PM +0100, Takashi Iwai wrote:
> Implement a new i915_audio_component_ops, get_eld().  It's called by
> the audio driver to fetch the current 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>
> ---
> v2->v3:
> * Track drm_connector for easier ELD retrieval, remove superfluous
>   audio_enabled flag instead
> * Back to av_mutex
> * Rebase to drm-next, update get_eld documentation for new kernel doc
> 
>  drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  include/drm/i915_component.h       | 14 +++++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 9aa83e71b792..eeac9f763110 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -521,6 +521,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;

This still has the problem that the eld might get ovewritten while we're
only holding av_mutex below. But I think that can/should be solved as part
of my probe vs. modeset locking rework.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	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);
>  }
> @@ -544,6 +548,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);
>  }
> @@ -703,6 +711,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,
> @@ -710,6 +751,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = {
>  	.codec_wake_override = i915_audio_component_codec_wake_override,
>  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
>  	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> +	.get_eld	= i915_audio_component_get_eld,
>  };
>  
>  static int i915_audio_component_bind(struct device *i915_dev,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab5c147fa9e9..fe58a5722b16 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -814,6 +814,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 fab13851f95a..b46fa0ef3005 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -65,6 +65,20 @@ struct i915_audio_component_ops {
>  	 * sample rate, it will call this function to set n/cts
>  	 */
>  	int (*sync_audio_rate)(struct device *, int port, int rate);
> +	/**
> +	 * @get_eld: fill the audio state and ELD bytes for the given port
> +	 *
> +	 * Called from audio driver to get the HDMI/DP audio state of the given
> +	 * digital port, and also fetch ELD bytes to the given pointer.
> +	 *
> +	 * It returns the byte size of the original ELD (not the actually
> +	 * copied size), zero for an invalid ELD, or a negative error code.
> +	 *
> +	 * Note that the returned size may be over @max_bytes.  Then it
> +	 * implies that only a part of ELD has been copied to the buffer.
> +	 */
> +	int (*get_eld)(struct device *, int port, bool *enabled,
> +		       unsigned char *buf, int max_bytes);
>  };
>  
>  /**
> -- 
> 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

[PATCH v3 2/4] 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>
---
v2->v3:
* Squashed previously two cleanup patches to a single patch

 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
 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, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15c6dc0b4f37..9dbc143cac27 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
 	u32 tmp;
 	int n;
 
@@ -655,21 +653,12 @@ 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);
-			if (!crtc) {
-				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
-				continue;
-			}
-			pipe = crtc->pipe;
-			break;
-		}
+	intel_encoder = dev_priv->dig_port_map[port];
+	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
+	    intel_encoder->base.crtc) {
+		crtc = to_intel_crtc(intel_encoder->base.crtc);
+		pipe = crtc->pipe;
 	}
-
 	if (pipe == INVALID_PIPE) {
 		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
 		mutex_unlock(&dev_priv->av_mutex);
@@ -716,27 +705,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 					unsigned char *buf, int max_bytes)
 {
 	struct drm_i915_private *dev_priv = dev_to_i915(dev);
-	struct drm_device *drm_dev = dev_priv->dev;
 	struct intel_encoder *intel_encoder;
 	struct intel_digital_port *intel_dig_port;
 	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
-			if (!*enabled)
-				break;
+		*enabled = intel_dig_port->audio_connector != NULL;
+		if (*enabled) {
 			eld = intel_dig_port->audio_connector->eld;
 			ret = drm_eld_size(eld);
 			memcpy(buf, eld, min(max_bytes, ret));
-			break;
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 76ce7c2960b6..59deb0d85533 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3295,6 +3295,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 bec443a629da..146f5da3acb1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6002,6 +6002,7 @@ intel_dp_init(struct drm_device *dev,
 	}
 
 	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 bdd462e7c690..c046017be786 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 void intel_hdmi_init(struct drm_device *dev,
 		     i915_reg_t 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;
@@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
 		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 = INVALID_MMIO_REG;
 
-- 
2.6.3

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

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

[Daniel Vetter]

On Fri, Dec 04, 2015 at 06:12:47PM +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.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v2->v3:
> * Squashed previously two cleanup patches to a single patch
> 
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
>  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, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15c6dc0b4f37..9dbc143cac27 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
>  	u32 tmp;
>  	int n;
>  
> @@ -655,21 +653,12 @@ 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);
> -			if (!crtc) {
> -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> -				continue;
> -			}
> -			pipe = crtc->pipe;
> -			break;
> -		}
> +	intel_encoder = dev_priv->dig_port_map[port];
> +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&

Is it really legit to call these functions with a port for which we don't
have an encoder? WARN_ON(!encoder) here and in the other places? Or would
that require some function for snd-hda to inquire i915 which ports are
enabled (which we probably should have to avoid registering audio ports
that aren't there)?

Otherwise lgtm.
-Daniel

> +	    intel_encoder->base.crtc) {
> +		crtc = to_intel_crtc(intel_encoder->base.crtc);
> +		pipe = crtc->pipe;
>  	}
> -
>  	if (pipe == INVALID_PIPE) {
>  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
>  		mutex_unlock(&dev_priv->av_mutex);
> @@ -716,27 +705,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  					unsigned char *buf, int max_bytes)
>  {
>  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> -	struct drm_device *drm_dev = dev_priv->dev;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_digital_port *intel_dig_port;
>  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> -			if (!*enabled)
> -				break;
> +		*enabled = intel_dig_port->audio_connector != NULL;
> +		if (*enabled) {
>  			eld = intel_dig_port->audio_connector->eld;
>  			ret = drm_eld_size(eld);
>  			memcpy(buf, eld, min(max_bytes, ret));
> -			break;
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 76ce7c2960b6..59deb0d85533 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3295,6 +3295,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 bec443a629da..146f5da3acb1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6002,6 +6002,7 @@ intel_dp_init(struct drm_device *dev,
>  	}
>  
>  	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 bdd462e7c690..c046017be786 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  void intel_hdmi_init(struct drm_device *dev,
>  		     i915_reg_t 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;
> @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
>  		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 = INVALID_MMIO_REG;
>  
> -- 
> 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 v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

On Mon, 07 Dec 2015 09:44:45 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 06:12:47PM +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.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v2->v3:
> > * Squashed previously two cleanup patches to a single patch
> > 
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> >  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, 17 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 15c6dc0b4f37..9dbc143cac27 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> >  	u32 tmp;
> >  	int n;
> >  
> > @@ -655,21 +653,12 @@ 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);
> > -			if (!crtc) {
> > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > -				continue;
> > -			}
> > -			pipe = crtc->pipe;
> > -			break;
> > -		}
> > +	intel_encoder = dev_priv->dig_port_map[port];
> > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> 
> Is it really legit to call these functions with a port for which we don't
> have an encoder? WARN_ON(!encoder) here and in the other places?

Currently there are little checks in the caller side.  So I guess only
a few of them deserve WARN_ON().  The empty encoder and empty crtc
would be good with WARN_ON(), as the port is supposed to be real. 
The HDMI check may be silent as is.

> Or would
> that require some function for snd-hda to inquire i915 which ports are
> enabled (which we probably should have to avoid registering audio ports
> that aren't there)?
> 
> Otherwise lgtm.

OK, I'll add WARN_ON() to them.


thanks,

Takashi

> -Daniel
> 
> > +	    intel_encoder->base.crtc) {
> > +		crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +		pipe = crtc->pipe;
> >  	}
> > -
> >  	if (pipe == INVALID_PIPE) {
> >  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> >  		mutex_unlock(&dev_priv->av_mutex);
> > @@ -716,27 +705,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> >  					unsigned char *buf, int max_bytes)
> >  {
> >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > -	struct drm_device *drm_dev = dev_priv->dev;
> >  	struct intel_encoder *intel_encoder;
> >  	struct intel_digital_port *intel_dig_port;
> >  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> > -			if (!*enabled)
> > -				break;
> > +		*enabled = intel_dig_port->audio_connector != NULL;
> > +		if (*enabled) {
> >  			eld = intel_dig_port->audio_connector->eld;
> >  			ret = drm_eld_size(eld);
> >  			memcpy(buf, eld, min(max_bytes, ret));
> > -			break;
> >  		}
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 76ce7c2960b6..59deb0d85533 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3295,6 +3295,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 bec443a629da..146f5da3acb1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6002,6 +6002,7 @@ intel_dp_init(struct drm_device *dev,
> >  	}
> >  
> >  	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 bdd462e7c690..c046017be786 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  void intel_hdmi_init(struct drm_device *dev,
> >  		     i915_reg_t 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;
> > @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
> >  		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 = INVALID_MMIO_REG;
> >  
> > -- 
> > 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 v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

On Mon, 07 Dec 2015 10:41:51 +0100,
Takashi Iwai wrote:
> 
> On Mon, 07 Dec 2015 09:44:45 +0100,
> Daniel Vetter wrote:
> > 
> > On Fri, Dec 04, 2015 at 06:12:47PM +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.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v2->v3:
> > > * Squashed previously two cleanup patches to a single patch
> > > 
> > >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> > >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> > >  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, 17 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 15c6dc0b4f37..9dbc143cac27 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> > >  	u32 tmp;
> > >  	int n;
> > >  
> > > @@ -655,21 +653,12 @@ 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);
> > > -			if (!crtc) {
> > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > -				continue;
> > > -			}
> > > -			pipe = crtc->pipe;
> > > -			break;
> > > -		}
> > > +	intel_encoder = dev_priv->dig_port_map[port];
> > > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> > 
> > Is it really legit to call these functions with a port for which we don't
> > have an encoder? WARN_ON(!encoder) here and in the other places?
> 
> Currently there are little checks in the caller side.  So I guess only
> a few of them deserve WARN_ON().  The empty encoder and empty crtc
> would be good with WARN_ON(), as the port is supposed to be real. 
> The HDMI check may be silent as is.
> 
> > Or would
> > that require some function for snd-hda to inquire i915 which ports are
> > enabled (which we probably should have to avoid registering audio ports
> > that aren't there)?
> > 
> > Otherwise lgtm.
> 
> OK, I'll add WARN_ON() to them.

It turned out that the encoder might be NULL for MST, as currently
it's not set properly yet.  So WARN_ON() will splat too much
unnecessarily.

Though, I rewrote a bit about the check and give a bit more messages.
Below is the revised patch.  So far, I have only this change, so I
hesitate to resubmit the whole patchset.  If a full patchset is
still preferred, let me know.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v4] drm/i915: Add reverse mapping between port and intel_encoder

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>
---
v3->v4: add a bit more verbose check for NULL encoder, etc.

 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_audio.c | 57 +++++++++++++++-----------------------
 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, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15c6dc0b4f37..9dbc143cac27 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1944,6 +1944,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 eeac9f763110..6380b2400448 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -636,15 +636,14 @@ 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 = INVALID_PIPE;
 	u32 tmp;
 	int n;
+	int err = 0;
 
 	/* HSW, BDW, SKL, KBL need this fix */
 	if (!IS_SKYLAKE(dev_priv) &&
@@ -655,26 +654,21 @@ 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);
-			if (!crtc) {
-				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
-				continue;
-			}
-			pipe = crtc->pipe;
-			break;
-		}
+	intel_encoder = dev_priv->dig_port_map[port];
+	if (!intel_encoder || !intel_encoder->base.crtc ||
+	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
+		err = -ENODEV;
+		goto unlock;
 	}
-
+	crtc = to_intel_crtc(intel_encoder->base.crtc);
+	pipe = crtc->pipe;
 	if (pipe == INVALID_PIPE) {
 		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
-		mutex_unlock(&dev_priv->av_mutex);
-		return -ENODEV;
+		err = -ENODEV;
+		goto unlock;
 	}
+
 	DRM_DEBUG_KMS("pipe %c connects port %c\n",
 				  pipe_name(pipe), port_name(port));
 	mode = &crtc->config->base.adjusted_mode;
@@ -687,8 +681,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 		tmp = I915_READ(HSW_AUD_CFG(pipe));
 		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
-		mutex_unlock(&dev_priv->av_mutex);
-		return 0;
+		goto unlock;
 	}
 
 	n = audio_config_get_n(mode, rate);
@@ -698,8 +691,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 		tmp = I915_READ(HSW_AUD_CFG(pipe));
 		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
-		mutex_unlock(&dev_priv->av_mutex);
-		return 0;
+		goto unlock;
 	}
 
 	/* 3. set the N/CTS/M */
@@ -707,8 +699,9 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 	tmp = audio_config_setup_n_reg(n, tmp);
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
 
+ unlock:
 	mutex_unlock(&dev_priv->av_mutex);
-	return 0;
+	return err;
 }
 
 static int i915_audio_component_get_eld(struct device *dev, int port,
@@ -716,27 +709,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 					unsigned char *buf, int max_bytes)
 {
 	struct drm_i915_private *dev_priv = dev_to_i915(dev);
-	struct drm_device *drm_dev = dev_priv->dev;
 	struct intel_encoder *intel_encoder;
 	struct intel_digital_port *intel_dig_port;
 	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
-			if (!*enabled)
-				break;
+		*enabled = intel_dig_port->audio_connector != NULL;
+		if (*enabled) {
 			eld = intel_dig_port->audio_connector->eld;
 			ret = drm_eld_size(eld);
 			memcpy(buf, eld, min(max_bytes, ret));
-			break;
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 76ce7c2960b6..59deb0d85533 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3295,6 +3295,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 e1ceff7ab265..e1456ead5c53 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6003,6 +6003,7 @@ intel_dp_init(struct drm_device *dev,
 	}
 
 	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 bdd462e7c690..c046017be786 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 void intel_hdmi_init(struct drm_device *dev,
 		     i915_reg_t 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;
@@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
 		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 = INVALID_MMIO_REG;
 
-- 
2.6.3

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

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

[Daniel Vetter]

On Tue, Dec 08, 2015 at 06:42:09PM +0100, Takashi Iwai wrote:
> On Mon, 07 Dec 2015 10:41:51 +0100,
> Takashi Iwai wrote:
> > 
> > On Mon, 07 Dec 2015 09:44:45 +0100,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Dec 04, 2015 at 06:12:47PM +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.
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > > v2->v3:
> > > > * Squashed previously two cleanup patches to a single patch
> > > > 
> > > >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> > > >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> > > >  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, 17 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 15c6dc0b4f37..9dbc143cac27 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> > > >  	u32 tmp;
> > > >  	int n;
> > > >  
> > > > @@ -655,21 +653,12 @@ 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);
> > > > -			if (!crtc) {
> > > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > > -				continue;
> > > > -			}
> > > > -			pipe = crtc->pipe;
> > > > -			break;
> > > > -		}
> > > > +	intel_encoder = dev_priv->dig_port_map[port];
> > > > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> > > 
> > > Is it really legit to call these functions with a port for which we don't
> > > have an encoder? WARN_ON(!encoder) here and in the other places?
> > 
> > Currently there are little checks in the caller side.  So I guess only
> > a few of them deserve WARN_ON().  The empty encoder and empty crtc
> > would be good with WARN_ON(), as the port is supposed to be real. 
> > The HDMI check may be silent as is.
> > 
> > > Or would
> > > that require some function for snd-hda to inquire i915 which ports are
> > > enabled (which we probably should have to avoid registering audio ports
> > > that aren't there)?
> > > 
> > > Otherwise lgtm.
> > 
> > OK, I'll add WARN_ON() to them.
> 
> It turned out that the encoder might be NULL for MST, as currently
> it's not set properly yet.  So WARN_ON() will splat too much
> unnecessarily.
> 
> Though, I rewrote a bit about the check and give a bit more messages.
> Below is the revised patch.  So far, I have only this change, so I
> hesitate to resubmit the whole patchset.  If a full patchset is
> still preferred, let me know.

Imo this note about mst should be in the commit message. lgtm otherwise.
Do you plan to prep a topic branch with all the patches that we could pull
into both snd and drm-intel trees?

Note that for 4.5 features drm-intel closes next week, so not that much
time left.
-Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH v4] drm/i915: Add reverse mapping between port and intel_encoder
> 
> 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>
> ---
> v3->v4: add a bit more verbose check for NULL encoder, etc.
> 
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_audio.c | 57 +++++++++++++++-----------------------
>  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, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15c6dc0b4f37..9dbc143cac27 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1944,6 +1944,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 eeac9f763110..6380b2400448 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -636,15 +636,14 @@ 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 = INVALID_PIPE;
>  	u32 tmp;
>  	int n;
> +	int err = 0;
>  
>  	/* HSW, BDW, SKL, KBL need this fix */
>  	if (!IS_SKYLAKE(dev_priv) &&
> @@ -655,26 +654,21 @@ 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);
> -			if (!crtc) {
> -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> -				continue;
> -			}
> -			pipe = crtc->pipe;
> -			break;
> -		}
> +	intel_encoder = dev_priv->dig_port_map[port];
> +	if (!intel_encoder || !intel_encoder->base.crtc ||
> +	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
> +		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> +		err = -ENODEV;
> +		goto unlock;
>  	}
> -
> +	crtc = to_intel_crtc(intel_encoder->base.crtc);
> +	pipe = crtc->pipe;
>  	if (pipe == INVALID_PIPE) {
>  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> -		mutex_unlock(&dev_priv->av_mutex);
> -		return -ENODEV;
> +		err = -ENODEV;
> +		goto unlock;
>  	}
> +
>  	DRM_DEBUG_KMS("pipe %c connects port %c\n",
>  				  pipe_name(pipe), port_name(port));
>  	mode = &crtc->config->base.adjusted_mode;
> @@ -687,8 +681,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  		tmp = I915_READ(HSW_AUD_CFG(pipe));
>  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> -		mutex_unlock(&dev_priv->av_mutex);
> -		return 0;
> +		goto unlock;
>  	}
>  
>  	n = audio_config_get_n(mode, rate);
> @@ -698,8 +691,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  		tmp = I915_READ(HSW_AUD_CFG(pipe));
>  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> -		mutex_unlock(&dev_priv->av_mutex);
> -		return 0;
> +		goto unlock;
>  	}
>  
>  	/* 3. set the N/CTS/M */
> @@ -707,8 +699,9 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	tmp = audio_config_setup_n_reg(n, tmp);
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> + unlock:
>  	mutex_unlock(&dev_priv->av_mutex);
> -	return 0;
> +	return err;
>  }
>  
>  static int i915_audio_component_get_eld(struct device *dev, int port,
> @@ -716,27 +709,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  					unsigned char *buf, int max_bytes)
>  {
>  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> -	struct drm_device *drm_dev = dev_priv->dev;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_digital_port *intel_dig_port;
>  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> -			if (!*enabled)
> -				break;
> +		*enabled = intel_dig_port->audio_connector != NULL;
> +		if (*enabled) {
>  			eld = intel_dig_port->audio_connector->eld;
>  			ret = drm_eld_size(eld);
>  			memcpy(buf, eld, min(max_bytes, ret));
> -			break;
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 76ce7c2960b6..59deb0d85533 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3295,6 +3295,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 e1ceff7ab265..e1456ead5c53 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6003,6 +6003,7 @@ intel_dp_init(struct drm_device *dev,
>  	}
>  
>  	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 bdd462e7c690..c046017be786 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  void intel_hdmi_init(struct drm_device *dev,
>  		     i915_reg_t 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;
> @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
>  		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 = INVALID_MMIO_REG;
>  
> -- 
> 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: [Intel-gfx] [PATCH v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder

[Takashi Iwai]

On Thu, 10 Dec 2015 10:38:14 +0100,
Daniel Vetter wrote:
> 
> On Tue, Dec 08, 2015 at 06:42:09PM +0100, Takashi Iwai wrote:
> > On Mon, 07 Dec 2015 10:41:51 +0100,
> > Takashi Iwai wrote:
> > > 
> > > On Mon, 07 Dec 2015 09:44:45 +0100,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Fri, Dec 04, 2015 at 06:12:47PM +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.
> > > > > 
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > ---
> > > > > v2->v3:
> > > > > * Squashed previously two cleanup patches to a single patch
> > > > > 
> > > > >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> > > > >  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, 17 insertions(+), 28 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 15c6dc0b4f37..9dbc143cac27 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> > > > >  	u32 tmp;
> > > > >  	int n;
> > > > >  
> > > > > @@ -655,21 +653,12 @@ 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);
> > > > > -			if (!crtc) {
> > > > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > > > -				continue;
> > > > > -			}
> > > > > -			pipe = crtc->pipe;
> > > > > -			break;
> > > > > -		}
> > > > > +	intel_encoder = dev_priv->dig_port_map[port];
> > > > > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> > > > 
> > > > Is it really legit to call these functions with a port for which we don't
> > > > have an encoder? WARN_ON(!encoder) here and in the other places?
> > > 
> > > Currently there are little checks in the caller side.  So I guess only
> > > a few of them deserve WARN_ON().  The empty encoder and empty crtc
> > > would be good with WARN_ON(), as the port is supposed to be real. 
> > > The HDMI check may be silent as is.
> > > 
> > > > Or would
> > > > that require some function for snd-hda to inquire i915 which ports are
> > > > enabled (which we probably should have to avoid registering audio ports
> > > > that aren't there)?
> > > > 
> > > > Otherwise lgtm.
> > > 
> > > OK, I'll add WARN_ON() to them.
> > 
> > It turned out that the encoder might be NULL for MST, as currently
> > it's not set properly yet.  So WARN_ON() will splat too much
> > unnecessarily.
> > 
> > Though, I rewrote a bit about the check and give a bit more messages.
> > Below is the revised patch.  So far, I have only this change, so I
> > hesitate to resubmit the whole patchset.  If a full patchset is
> > still preferred, let me know.
> 
> Imo this note about mst should be in the commit message. lgtm otherwise.

OK, I'll update the changelog.

> Do you plan to prep a topic branch with all the patches that we could pull
> into both snd and drm-intel trees?

Yes, currently test/hdmi-jack branch contains these based on drm-next
and some sound updates.

> Note that for 4.5 features drm-intel closes next week, so not that much
> time left.

Yeah, I'd like to merge this stuff soonishly, too.


thanks,

Takashi

> -Daniel
> 
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH v4] drm/i915: Add reverse mapping between port and intel_encoder
> > 
> > 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>
> > ---
> > v3->v4: add a bit more verbose check for NULL encoder, etc.
> > 
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 57 +++++++++++++++-----------------------
> >  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, 28 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 15c6dc0b4f37..9dbc143cac27 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1944,6 +1944,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 eeac9f763110..6380b2400448 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -636,15 +636,14 @@ 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 = INVALID_PIPE;
> >  	u32 tmp;
> >  	int n;
> > +	int err = 0;
> >  
> >  	/* HSW, BDW, SKL, KBL need this fix */
> >  	if (!IS_SKYLAKE(dev_priv) &&
> > @@ -655,26 +654,21 @@ 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);
> > -			if (!crtc) {
> > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > -				continue;
> > -			}
> > -			pipe = crtc->pipe;
> > -			break;
> > -		}
> > +	intel_encoder = dev_priv->dig_port_map[port];
> > +	if (!intel_encoder || !intel_encoder->base.crtc ||
> > +	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
> > +		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > +		err = -ENODEV;
> > +		goto unlock;
> >  	}
> > -
> > +	crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +	pipe = crtc->pipe;
> >  	if (pipe == INVALID_PIPE) {
> >  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> > -		mutex_unlock(&dev_priv->av_mutex);
> > -		return -ENODEV;
> > +		err = -ENODEV;
> > +		goto unlock;
> >  	}
> > +
> >  	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> >  				  pipe_name(pipe), port_name(port));
> >  	mode = &crtc->config->base.adjusted_mode;
> > @@ -687,8 +681,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> >  		tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > -		mutex_unlock(&dev_priv->av_mutex);
> > -		return 0;
> > +		goto unlock;
> >  	}
> >  
> >  	n = audio_config_get_n(mode, rate);
> > @@ -698,8 +691,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> >  		tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > -		mutex_unlock(&dev_priv->av_mutex);
> > -		return 0;
> > +		goto unlock;
> >  	}
> >  
> >  	/* 3. set the N/CTS/M */
> > @@ -707,8 +699,9 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> >  	tmp = audio_config_setup_n_reg(n, tmp);
> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  
> > + unlock:
> >  	mutex_unlock(&dev_priv->av_mutex);
> > -	return 0;
> > +	return err;
> >  }
> >  
> >  static int i915_audio_component_get_eld(struct device *dev, int port,
> > @@ -716,27 +709,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> >  					unsigned char *buf, int max_bytes)
> >  {
> >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > -	struct drm_device *drm_dev = dev_priv->dev;
> >  	struct intel_encoder *intel_encoder;
> >  	struct intel_digital_port *intel_dig_port;
> >  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> > -			if (!*enabled)
> > -				break;
> > +		*enabled = intel_dig_port->audio_connector != NULL;
> > +		if (*enabled) {
> >  			eld = intel_dig_port->audio_connector->eld;
> >  			ret = drm_eld_size(eld);
> >  			memcpy(buf, eld, min(max_bytes, ret));
> > -			break;
> >  		}
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 76ce7c2960b6..59deb0d85533 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3295,6 +3295,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 e1ceff7ab265..e1456ead5c53 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6003,6 +6003,7 @@ intel_dp_init(struct drm_device *dev,
> >  	}
> >  
> >  	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 bdd462e7c690..c046017be786 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  void intel_hdmi_init(struct drm_device *dev,
> >  		     i915_reg_t 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;
> > @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
> >  		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 = INVALID_MMIO_REG;
> >  
> > -- 
> > 2.6.3
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 

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

[Daniel Vetter]

On Thu, Dec 10, 2015 at 10:47:50AM +0100, Takashi Iwai wrote:
> On Thu, 10 Dec 2015 10:38:14 +0100,
> Daniel Vetter wrote:
> > 
> > On Tue, Dec 08, 2015 at 06:42:09PM +0100, Takashi Iwai wrote:
> > > On Mon, 07 Dec 2015 10:41:51 +0100,
> > > Takashi Iwai wrote:
> > > > 
> > > > On Mon, 07 Dec 2015 09:44:45 +0100,
> > > > Daniel Vetter wrote:
> > > > > 
> > > > > On Fri, Dec 04, 2015 at 06:12:47PM +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.
> > > > > > 
> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > ---
> > > > > > v2->v3:
> > > > > > * Squashed previously two cleanup patches to a single patch
> > > > > > 
> > > > > >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> > > > > >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> > > > > >  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, 17 insertions(+), 28 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 15c6dc0b4f37..9dbc143cac27 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -1944,6 +1944,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 eeac9f763110..05f08b445dd6 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> > > > > >  	u32 tmp;
> > > > > >  	int n;
> > > > > >  
> > > > > > @@ -655,21 +653,12 @@ 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);
> > > > > > -			if (!crtc) {
> > > > > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > > > > -				continue;
> > > > > > -			}
> > > > > > -			pipe = crtc->pipe;
> > > > > > -			break;
> > > > > > -		}
> > > > > > +	intel_encoder = dev_priv->dig_port_map[port];
> > > > > > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> > > > > 
> > > > > Is it really legit to call these functions with a port for which we don't
> > > > > have an encoder? WARN_ON(!encoder) here and in the other places?
> > > > 
> > > > Currently there are little checks in the caller side.  So I guess only
> > > > a few of them deserve WARN_ON().  The empty encoder and empty crtc
> > > > would be good with WARN_ON(), as the port is supposed to be real. 
> > > > The HDMI check may be silent as is.
> > > > 
> > > > > Or would
> > > > > that require some function for snd-hda to inquire i915 which ports are
> > > > > enabled (which we probably should have to avoid registering audio ports
> > > > > that aren't there)?
> > > > > 
> > > > > Otherwise lgtm.
> > > > 
> > > > OK, I'll add WARN_ON() to them.
> > > 
> > > It turned out that the encoder might be NULL for MST, as currently
> > > it's not set properly yet.  So WARN_ON() will splat too much
> > > unnecessarily.
> > > 
> > > Though, I rewrote a bit about the check and give a bit more messages.
> > > Below is the revised patch.  So far, I have only this change, so I
> > > hesitate to resubmit the whole patchset.  If a full patchset is
> > > still preferred, let me know.
> > 
> > Imo this note about mst should be in the commit message. lgtm otherwise.
> 
> OK, I'll update the changelog.
> 
> > Do you plan to prep a topic branch with all the patches that we could pull
> > into both snd and drm-intel trees?
> 
> Yes, currently test/hdmi-jack branch contains these based on drm-next
> and some sound updates.
> 
> > Note that for 4.5 features drm-intel closes next week, so not that much
> > time left.
> 
> Yeah, I'd like to merge this stuff soonishly, too.

Great. When it's all ready can you pls send me a pull req?

Thanks, Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> > -Daniel
> > 
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH v4] drm/i915: Add reverse mapping between port and intel_encoder
> > > 
> > > 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>
> > > ---
> > > v3->v4: add a bit more verbose check for NULL encoder, etc.
> > > 
> > >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> > >  drivers/gpu/drm/i915/intel_audio.c | 57 +++++++++++++++-----------------------
> > >  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, 28 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 15c6dc0b4f37..9dbc143cac27 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1944,6 +1944,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 eeac9f763110..6380b2400448 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -636,15 +636,14 @@ 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 = INVALID_PIPE;
> > >  	u32 tmp;
> > >  	int n;
> > > +	int err = 0;
> > >  
> > >  	/* HSW, BDW, SKL, KBL need this fix */
> > >  	if (!IS_SKYLAKE(dev_priv) &&
> > > @@ -655,26 +654,21 @@ 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);
> > > -			if (!crtc) {
> > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > -				continue;
> > > -			}
> > > -			pipe = crtc->pipe;
> > > -			break;
> > > -		}
> > > +	intel_encoder = dev_priv->dig_port_map[port];
> > > +	if (!intel_encoder || !intel_encoder->base.crtc ||
> > > +	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
> > > +		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > +		err = -ENODEV;
> > > +		goto unlock;
> > >  	}
> > > -
> > > +	crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > +	pipe = crtc->pipe;
> > >  	if (pipe == INVALID_PIPE) {
> > >  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> > > -		mutex_unlock(&dev_priv->av_mutex);
> > > -		return -ENODEV;
> > > +		err = -ENODEV;
> > > +		goto unlock;
> > >  	}
> > > +
> > >  	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > >  				  pipe_name(pipe), port_name(port));
> > >  	mode = &crtc->config->base.adjusted_mode;
> > > @@ -687,8 +681,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > >  		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > >  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > >  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > -		mutex_unlock(&dev_priv->av_mutex);
> > > -		return 0;
> > > +		goto unlock;
> > >  	}
> > >  
> > >  	n = audio_config_get_n(mode, rate);
> > > @@ -698,8 +691,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > >  		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > >  		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > >  		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > -		mutex_unlock(&dev_priv->av_mutex);
> > > -		return 0;
> > > +		goto unlock;
> > >  	}
> > >  
> > >  	/* 3. set the N/CTS/M */
> > > @@ -707,8 +699,9 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > >  	tmp = audio_config_setup_n_reg(n, tmp);
> > >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >  
> > > + unlock:
> > >  	mutex_unlock(&dev_priv->av_mutex);
> > > -	return 0;
> > > +	return err;
> > >  }
> > >  
> > >  static int i915_audio_component_get_eld(struct device *dev, int port,
> > > @@ -716,27 +709,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> > >  					unsigned char *buf, int max_bytes)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > -	struct drm_device *drm_dev = dev_priv->dev;
> > >  	struct intel_encoder *intel_encoder;
> > >  	struct intel_digital_port *intel_dig_port;
> > >  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> > > -			if (!*enabled)
> > > -				break;
> > > +		*enabled = intel_dig_port->audio_connector != NULL;
> > > +		if (*enabled) {
> > >  			eld = intel_dig_port->audio_connector->eld;
> > >  			ret = drm_eld_size(eld);
> > >  			memcpy(buf, eld, min(max_bytes, ret));
> > > -			break;
> > >  		}
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 76ce7c2960b6..59deb0d85533 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -3295,6 +3295,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 e1ceff7ab265..e1456ead5c53 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -6003,6 +6003,7 @@ intel_dp_init(struct drm_device *dev,
> > >  	}
> > >  
> > >  	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 bdd462e7c690..c046017be786 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >  void intel_hdmi_init(struct drm_device *dev,
> > >  		     i915_reg_t 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;
> > > @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
> > >  		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 = INVALID_MMIO_REG;
> > >  
> > > -- 
> > > 2.6.3
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > 

-- 
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 v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder

[Jani Nikula]

On Fri, 04 Dec 2015, Takashi Iwai <tiwai@suse.de> 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.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v2->v3:
> * Squashed previously two cleanup patches to a single patch
>
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
>  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, 17 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15c6dc0b4f37..9dbc143cac27 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1944,6 +1944,8 @@ struct drm_i915_private {
>  	/* perform PHY state sanity checks? */
>  	bool chv_phy_assert[2];
>  
> +	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> +

The DP MST code seems to use dev_priv->hotplug.irq_port[] for the
mapping. All of this should probably be consolidated in the end, but I
don't insist you do it now.

BR,
Jani.


>  	/*
>  	 * 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 eeac9f763110..05f08b445dd6 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
>  	u32 tmp;
>  	int n;
>  
> @@ -655,21 +653,12 @@ 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);
> -			if (!crtc) {
> -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> -				continue;
> -			}
> -			pipe = crtc->pipe;
> -			break;
> -		}
> +	intel_encoder = dev_priv->dig_port_map[port];
> +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> +	    intel_encoder->base.crtc) {
> +		crtc = to_intel_crtc(intel_encoder->base.crtc);
> +		pipe = crtc->pipe;
>  	}
> -
>  	if (pipe == INVALID_PIPE) {
>  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
>  		mutex_unlock(&dev_priv->av_mutex);
> @@ -716,27 +705,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  					unsigned char *buf, int max_bytes)
>  {
>  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> -	struct drm_device *drm_dev = dev_priv->dev;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_digital_port *intel_dig_port;
>  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> -			if (!*enabled)
> -				break;
> +		*enabled = intel_dig_port->audio_connector != NULL;
> +		if (*enabled) {
>  			eld = intel_dig_port->audio_connector->eld;
>  			ret = drm_eld_size(eld);
>  			memcpy(buf, eld, min(max_bytes, ret));
> -			break;
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 76ce7c2960b6..59deb0d85533 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3295,6 +3295,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 bec443a629da..146f5da3acb1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6002,6 +6002,7 @@ intel_dp_init(struct drm_device *dev,
>  	}
>  
>  	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 bdd462e7c690..c046017be786 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  void intel_hdmi_init(struct drm_device *dev,
>  		     i915_reg_t 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;
> @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
>  		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 = INVALID_MMIO_REG;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Mon, 07 Dec 2015 11:30:05 +0100,
Jani Nikula wrote:
> 
> On Fri, 04 Dec 2015, Takashi Iwai <tiwai@suse.de> 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.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v2->v3:
> > * Squashed previously two cleanup patches to a single patch
> >
> >  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++---------------------------
> >  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, 17 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 15c6dc0b4f37..9dbc143cac27 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1944,6 +1944,8 @@ struct drm_i915_private {
> >  	/* perform PHY state sanity checks? */
> >  	bool chv_phy_assert[2];
> >  
> > +	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> > +
> 
> The DP MST code seems to use dev_priv->hotplug.irq_port[] for the
> mapping. All of this should probably be consolidated in the end, but I
> don't insist you do it now.

Yes, it's a bit redundant, indeed.  IMO, irq_port[] is mislabeled as a
generic usage, so we'd need rename / unify things in future.


Takashi

> 
> BR,
> Jani.
> 
> 
> >  	/*
> >  	 * 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 eeac9f763110..05f08b445dd6 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -636,13 +636,11 @@ 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 = INVALID_PIPE;
> >  	u32 tmp;
> >  	int n;
> >  
> > @@ -655,21 +653,12 @@ 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);
> > -			if (!crtc) {
> > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > -				continue;
> > -			}
> > -			pipe = crtc->pipe;
> > -			break;
> > -		}
> > +	intel_encoder = dev_priv->dig_port_map[port];
> > +	if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&
> > +	    intel_encoder->base.crtc) {
> > +		crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +		pipe = crtc->pipe;
> >  	}
> > -
> >  	if (pipe == INVALID_PIPE) {
> >  		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> >  		mutex_unlock(&dev_priv->av_mutex);
> > @@ -716,27 +705,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> >  					unsigned char *buf, int max_bytes)
> >  {
> >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > -	struct drm_device *drm_dev = dev_priv->dev;
> >  	struct intel_encoder *intel_encoder;
> >  	struct intel_digital_port *intel_dig_port;
> >  	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_encoder = dev_priv->dig_port_map[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_connector != NULL;
> > -			if (!*enabled)
> > -				break;
> > +		*enabled = intel_dig_port->audio_connector != NULL;
> > +		if (*enabled) {
> >  			eld = intel_dig_port->audio_connector->eld;
> >  			ret = drm_eld_size(eld);
> >  			memcpy(buf, eld, min(max_bytes, ret));
> > -			break;
> >  		}
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 76ce7c2960b6..59deb0d85533 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3295,6 +3295,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 bec443a629da..146f5da3acb1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6002,6 +6002,7 @@ intel_dp_init(struct drm_device *dev,
> >  	}
> >  
> >  	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 bdd462e7c690..c046017be786 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  void intel_hdmi_init(struct drm_device *dev,
> >  		     i915_reg_t 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;
> > @@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev,
> >  		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 = INVALID_MMIO_REG;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

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

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

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

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v2->v3:
* Adapt the earlier applied preliminary patches
* Direct invocation of get_eld now instead of deferred call, as we
  have no longer deadlocks

 sound/pci/hda/patch_hdmi.c | 111 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 97 insertions(+), 14 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 85342d261043..61f004a73590 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 */
@@ -1437,6 +1438,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
  */
@@ -1582,7 +1594,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;
@@ -1642,6 +1656,56 @@ 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) {
+			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;
+
+	if (codec_has_acomp(codec)) {
+		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 =
@@ -1777,17 +1841,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
  */
@@ -2092,6 +2145,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";
@@ -2102,6 +2179,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 (codec_has_acomp(codec))
+		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",
@@ -2197,8 +2276,10 @@ static int generic_hdmi_init(struct hda_codec *codec)
 		hda_nid_t pin_nid = per_pin->pin_nid;
 
 		hdmi_init_pin(codec, pin_nid);
-		snd_hda_jack_detect_enable_callback(codec, pin_nid,
-			codec->jackpoll_interval > 0 ? jack_callback : NULL);
+		if (!codec_has_acomp(codec))
+			snd_hda_jack_detect_enable_callback(codec, pin_nid,
+				codec->jackpoll_interval > 0 ?
+				jack_callback : NULL);
 	}
 	return 0;
 }
@@ -2228,6 +2309,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);
-- 
2.6.3

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

[PATCH v3 4/4] 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 61f004a73590..1ef0c6959e75 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1438,17 +1438,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
  */
@@ -1660,38 +1649,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)
@@ -1857,7 +1844,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;
 
@@ -1876,10 +1862,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 v3 4/4] ALSA: hda - Move audio component accesses to hdac_i915.c

[Vinod Koul]

On Fri, Dec 04, 2015 at 06:12:49PM +0100, Takashi Iwai wrote:
> 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>

Reviewed-by: Vinod Koul <vinod.koul@intel.com>

-- 
~Vinod
> ---
>  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 61f004a73590..1ef0c6959e75 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1438,17 +1438,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
>   */
> @@ -1660,38 +1649,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)
> @@ -1857,7 +1844,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;
>  
> @@ -1876,10 +1862,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