[PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle

[PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c  | 95 ++++++++++++----------------------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 29 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ed37407..1eef55c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3764,27 +3764,10 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	return drm_get_edid(connector, adapter);
 }
 
-static int
-intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
-{
-	struct intel_connector *intel_connector = to_intel_connector(connector);
-
-	/* use cached edid if we have one */
-	if (intel_connector->edid) {
-		/* invalid edid */
-		if (IS_ERR(intel_connector->edid))
-			return 0;
-
-		return intel_connector_update_modes(connector,
-						    intel_connector->edid);
-	}
-
-	return intel_ddc_get_modes(connector, adapter);
-}
-
 static enum drm_connector_status
 intel_dp_detect(struct drm_connector *connector, bool force)
 {
+	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -3795,21 +3778,20 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct edid *edid = NULL;
 	bool ret;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
+	kfree(intel_connector->detect_edid);
+	intel_connector->detect_edid = NULL;
 
 	if (intel_dp->is_mst) {
 		/* MST devices are disconnected from a monitor POV */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
-		goto out;
+		return connector_status_disconnected;
 	}
 
-	intel_dp->has_audio = false;
+	power_domain = intel_display_port_power_domain(intel_encoder);
+	intel_display_power_get(dev_priv, power_domain);
 
 	if (HAS_PCH_SPLIT(dev))
 		status = ironlake_dp_detect(intel_dp);
@@ -3831,15 +3813,13 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
+	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
+	intel_connector->detect_edid = edid;
+
+	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
 		intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
-	} else {
-		edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-		if (edid) {
-			intel_dp->has_audio = drm_detect_monitor_audio(edid);
-			kfree(edid);
-		}
-	}
+	else
+		intel_dp->has_audio = drm_detect_monitor_audio(edid);
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP)
 		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
@@ -3852,61 +3832,41 @@ out:
 
 static int intel_dp_get_modes(struct drm_connector *connector)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	int ret;
-
-	/* We should parse the EDID data and find out if it has an audio sink
-	 */
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	struct edid *edid;
 
-	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
-	intel_display_power_put(dev_priv, power_domain);
-	if (ret)
-		return ret;
+	edid = intel_connector->detect_edid;
+	if (edid) {
+		int ret = intel_connector_update_modes(connector, edid);
+		if (ret)
+			return ret;
+	}
 
 	/* if eDP has no EDID, fall back to fixed mode */
-	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+	if (is_edp(intel_attached_dp(connector)) &&
+	    intel_connector->panel.fixed_mode) {
 		struct drm_display_mode *mode;
-		mode = drm_mode_duplicate(dev,
+
+		mode = drm_mode_duplicate(connector->dev,
 					  intel_connector->panel.fixed_mode);
 		if (mode) {
 			drm_mode_probed_add(connector, mode);
 			return 1;
 		}
 	}
+
 	return 0;
 }
 
 static bool
 intel_dp_detect_audio(struct drm_connector *connector)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-	if (edid) {
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid)
 		has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
 
 	return has_audio;
 }
@@ -4004,6 +3964,7 @@ intel_dp_connector_destroy(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
+	kfree(intel_connector->detect_edid);
 	if (!IS_ERR_OR_NULL(intel_connector->edid))
 		kfree(intel_connector->edid);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a031bbf..bb324ac 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -205,6 +205,7 @@ struct intel_connector {
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
 	struct edid *edid;
+	struct edid *detect_edid;
 
 	/* since POLL and HPD connectors may use the same HPD line keep the native
 	   state of connector->polled in case hotplug storm detection changes it */
-- 
1.9.1

[PATCH 2/2] drm/i915/hdmi: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 68 ++++++++++++---------------------------
 1 file changed, 21 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9169786..f765527 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -988,17 +988,17 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 			    intel_gmbus_get_adapter(dev_priv,
 						    intel_hdmi->ddc_bus));
 
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
-			status = connector_status_connected;
-			if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
-				intel_hdmi->has_hdmi_sink =
-						drm_detect_hdmi_monitor(edid);
-			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
-			intel_hdmi->rgb_quant_range_selectable =
-				drm_rgb_quant_range_selectable(edid);
-		}
-		kfree(edid);
+	kfree(to_intel_connector(connector)->detect_edid);
+	to_intel_connector(connector)->detect_edid = edid;
+
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+		status = connector_status_connected;
+		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
+			intel_hdmi->has_hdmi_sink =
+				drm_detect_hdmi_monitor(edid);
+		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
+		intel_hdmi->rgb_quant_range_selectable =
+			drm_rgb_quant_range_selectable(edid);
 	}
 
 	if (status == connector_status_connected) {
@@ -1015,51 +1015,24 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 
 static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	int ret;
-
-	/* We should parse the EDID data and find out if it's an HDMI sink so
-	 * we can send audio to it.
-	 */
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	ret = intel_ddc_get_modes(connector,
-				   intel_gmbus_get_adapter(dev_priv,
-							   intel_hdmi->ddc_bus));
+	struct edid *edid;
 
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid == NULL)
+		return 0;
 
-	return ret;
+	return intel_connector_update_modes(connector, edid);
 }
 
 static bool
 intel_hdmi_detect_audio(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL)
-			has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
+		has_audio = drm_detect_monitor_audio(edid);
 
 	return has_audio;
 }
@@ -1479,6 +1452,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 static void intel_hdmi_destroy(struct drm_connector *connector)
 {
+	kfree(to_intel_connector(connector)->detect_edid);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
-- 
1.9.1

Re: [PATCH 2/2] drm/i915/hdmi: Cache EDID for a detection cycle

[Chris Wilson]

On Tue, Aug 12, 2014 at 09:36:04AM +0100, Chris Wilson wrote:
> As we may query the edid multiple times following a detect, record the
> EDID found during output discovery and reuse it. This is a separate
> issue from caching the output EDID across detection cycles.

As reference, for a couple of DP monitors attached,
"time xrandr" goes from 0.268s to 0.142s
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle

[Ville Syrjälä]

On Tue, Aug 12, 2014 at 09:36:03AM +0100, Chris Wilson wrote:
> As we may query the edid multiple times following a detect, record the
> EDID found during output discovery and reuse it. This is a separate
> issue from caching the output EDID across detection cycles.

My only real concern here is what happens when someone forces the
connector to connected. Given you only populate detect_edid in
->detect() we wouldn't be able to get the modes from the EDID in
that case.

I guess one solution would be to implement the ->force() hook and
attempt the edid read there. Looking at the code we should always do
a ->detect() or ->force() before ->get_modes().

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 95 ++++++++++++----------------------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 29 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ed37407..1eef55c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3764,27 +3764,10 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	return drm_get_edid(connector, adapter);
>  }
>  
> -static int
> -intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
> -{
> -	struct intel_connector *intel_connector = to_intel_connector(connector);
> -
> -	/* use cached edid if we have one */
> -	if (intel_connector->edid) {
> -		/* invalid edid */
> -		if (IS_ERR(intel_connector->edid))
> -			return 0;
> -
> -		return intel_connector_update_modes(connector,
> -						    intel_connector->edid);
> -	}
> -
> -	return intel_ddc_get_modes(connector, adapter);
> -}
> -
>  static enum drm_connector_status
>  intel_dp_detect(struct drm_connector *connector, bool force)
>  {
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> @@ -3795,21 +3778,20 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  	struct edid *edid = NULL;
>  	bool ret;
>  
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> -
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
> +	kfree(intel_connector->detect_edid);
> +	intel_connector->detect_edid = NULL;
>  
>  	if (intel_dp->is_mst) {
>  		/* MST devices are disconnected from a monitor POV */
>  		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> -		status = connector_status_disconnected;
> -		goto out;
> +		return connector_status_disconnected;
>  	}
>  
> -	intel_dp->has_audio = false;
> +	power_domain = intel_display_port_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
>  
>  	if (HAS_PCH_SPLIT(dev))
>  		status = ironlake_dp_detect(intel_dp);
> @@ -3831,15 +3813,13 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		goto out;
>  	}
>  
> -	if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
> +	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> +	intel_connector->detect_edid = edid;
> +
> +	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
>  		intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
> -	} else {
> -		edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> -		if (edid) {
> -			intel_dp->has_audio = drm_detect_monitor_audio(edid);
> -			kfree(edid);
> -		}
> -	}
> +	else
> +		intel_dp->has_audio = drm_detect_monitor_audio(edid);
>  
>  	if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> @@ -3852,61 +3832,41 @@ out:
>  
>  static int intel_dp_get_modes(struct drm_connector *connector)
>  {
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
> -	struct drm_device *dev = connector->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum intel_display_power_domain power_domain;
> -	int ret;
> -
> -	/* We should parse the EDID data and find out if it has an audio sink
> -	 */
> -
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> +	struct edid *edid;
>  
> -	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
> -	intel_display_power_put(dev_priv, power_domain);
> -	if (ret)
> -		return ret;
> +	edid = intel_connector->detect_edid;
> +	if (edid) {
> +		int ret = intel_connector_update_modes(connector, edid);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* if eDP has no EDID, fall back to fixed mode */
> -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> +	if (is_edp(intel_attached_dp(connector)) &&
> +	    intel_connector->panel.fixed_mode) {
>  		struct drm_display_mode *mode;
> -		mode = drm_mode_duplicate(dev,
> +
> +		mode = drm_mode_duplicate(connector->dev,
>  					  intel_connector->panel.fixed_mode);
>  		if (mode) {
>  			drm_mode_probed_add(connector, mode);
>  			return 1;
>  		}
>  	}
> +
>  	return 0;
>  }
>  
>  static bool
>  intel_dp_detect_audio(struct drm_connector *connector)
>  {
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> -	struct drm_device *dev = connector->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum intel_display_power_domain power_domain;
> -	struct edid *edid;
>  	bool has_audio = false;
> +	struct edid *edid;
>  
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> -
> -	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> -	if (edid) {
> +	edid = to_intel_connector(connector)->detect_edid;
> +	if (edid)
>  		has_audio = drm_detect_monitor_audio(edid);
> -		kfree(edid);
> -	}
> -
> -	intel_display_power_put(dev_priv, power_domain);
>  
>  	return has_audio;
>  }
> @@ -4004,6 +3964,7 @@ intel_dp_connector_destroy(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  
> +	kfree(intel_connector->detect_edid);
>  	if (!IS_ERR_OR_NULL(intel_connector->edid))
>  		kfree(intel_connector->edid);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a031bbf..bb324ac 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -205,6 +205,7 @@ struct intel_connector {
>  
>  	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
>  	struct edid *edid;
> +	struct edid *detect_edid;
>  
>  	/* since POLL and HPD connectors may use the same HPD line keep the native
>  	   state of connector->polled in case hotplug storm detection changes it */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle

[Chris Wilson]

On Mon, Sep 01, 2014 at 03:05:26PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 12, 2014 at 09:36:03AM +0100, Chris Wilson wrote:
> > As we may query the edid multiple times following a detect, record the
> > EDID found during output discovery and reuse it. This is a separate
> > issue from caching the output EDID across detection cycles.
> 
> My only real concern here is what happens when someone forces the
> connector to connected. Given you only populate detect_edid in
> ->detect() we wouldn't be able to get the modes from the EDID in
> that case.
> 
> I guess one solution would be to implement the ->force() hook and
> attempt the edid read there. Looking at the code we should always do
> a ->detect() or ->force() before ->get_modes().

That should fix our audio detection in those cases as well. Sounds good.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/3] drm/i915/dp: Refactor common eDP lid detection

[Chris Wilson]

Both gmch and pch detection routines used the exact same routine for
eDP, so de-duplicate.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 048eb2bec04c..42de5cc86b0d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3725,20 +3725,24 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 }
 
 static enum drm_connector_status
+edp_detect(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	enum drm_connector_status status;
+
+	status = intel_panel_detect(dev);
+	if (status == connector_status_unknown)
+		status = connector_status_connected;
+
+	return status;
+}
+
+static enum drm_connector_status
 ironlake_dp_detect(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	enum drm_connector_status status;
-
-	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp)) {
-		status = intel_panel_detect(dev);
-		if (status == connector_status_unknown)
-			status = connector_status_connected;
-		return status;
-	}
 
 	if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
 		return connector_status_disconnected;
@@ -3794,16 +3798,6 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	int ret;
 
-	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp)) {
-		enum drm_connector_status status;
-
-		status = intel_panel_detect(dev);
-		if (status == connector_status_unknown)
-			status = connector_status_connected;
-		return status;
-	}
-
 	ret = g4x_digital_port_connected(dev, intel_dig_port);
 	if (ret == -EINVAL)
 		return connector_status_unknown;
@@ -3877,11 +3871,13 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	intel_dp->has_audio = false;
 
-	if (HAS_PCH_SPLIT(dev))
+	/* Can't disconnect eDP, but you can close the lid... */
+	if (is_edp(intel_dp))
+		status = edp_detect(intel_dp);
+	else if (HAS_PCH_SPLIT(dev))
 		status = ironlake_dp_detect(intel_dp);
 	else
 		status = g4x_dp_detect(intel_dp);
-
 	if (status != connector_status_connected)
 		goto out;
 
-- 
2.1.0

[PATCH 2/3] drm/i915/dp: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

v2: Implement connector->force() callback so that edid is associated
with the connector for user overrides as well (Ville)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 156 ++++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h |   1 +
 2 files changed, 90 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 42de5cc86b0d..69ff82a7ec4c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3808,9 +3808,9 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 }
 
 static struct edid *
-intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+intel_dp_get_edid(struct intel_dp *intel_dp)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 
 	/* use cached edid if we have one */
 	if (intel_connector->edid) {
@@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			return NULL;
 
 		return drm_edid_duplicate(intel_connector->edid);
-	}
+	} else
+		return drm_get_edid(&intel_connector->base,
+				    &intel_dp->aux.ddc);
+}
 
-	return drm_get_edid(connector, adapter);
+static void
+intel_dp_set_edid(struct intel_dp *intel_dp)
+{
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct edid *edid;
+
+	edid = intel_dp_get_edid(intel_dp);
+	intel_connector->detect_edid = edid;
+
+	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
+		intel_dp->has_audio = intel_dp->force_audio == HDMI_AUDIO_ON;
+	else
+		intel_dp->has_audio = drm_detect_monitor_audio(edid);
 }
 
-static int
-intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
+static void
+intel_dp_unset_edid(struct intel_dp *intel_dp)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 
-	/* use cached edid if we have one */
-	if (intel_connector->edid) {
-		/* invalid edid */
-		if (IS_ERR(intel_connector->edid))
-			return 0;
+	kfree(intel_connector->detect_edid);
+	intel_connector->detect_edid = NULL;
 
-		return intel_connector_update_modes(connector,
-						    intel_connector->edid);
-	}
+	intel_dp->has_audio = false;
+}
 
-	return intel_ddc_get_modes(connector, adapter);
+static enum intel_display_power_domain
+intel_dp_power_get(struct intel_dp *dp)
+{
+	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(to_i915(encoder->base.dev), power_domain);
+
+	return power_domain;
+}
+
+static void
+intel_dp_power_put(struct intel_dp *dp,
+		   enum intel_display_power_domain power_domain)
+{
+	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
+	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
 static enum drm_connector_status
@@ -3849,27 +3877,22 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid = NULL;
 	bool ret;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
+	intel_dp_unset_edid(intel_dp);
 
 	if (intel_dp->is_mst) {
 		/* MST devices are disconnected from a monitor POV */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
-		goto out;
+		return connector_status_disconnected;
 	}
 
-	intel_dp->has_audio = false;
+	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
 	if (is_edp(intel_dp))
@@ -3893,82 +3916,78 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
-		intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
-	} else {
-		edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-		if (edid) {
-			intel_dp->has_audio = drm_detect_monitor_audio(edid);
-			kfree(edid);
-		}
-	}
+	intel_dp_set_edid(intel_dp);
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP)
 		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 	status = connector_status_connected;
 
 out:
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_dp, power_domain);
 	return status;
 }
 
-static int intel_dp_get_modes(struct drm_connector *connector)
+static void
+intel_dp_force(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	enum intel_display_power_domain power_domain;
-	int ret;
 
-	/* We should parse the EDID data and find out if it has an audio sink
-	 */
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+	intel_dp_unset_edid(intel_dp);
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	if (connector->status != connector_status_connected)
+		return;
 
-	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
-	intel_display_power_put(dev_priv, power_domain);
-	if (ret)
-		return ret;
+	power_domain = intel_dp_power_get(intel_dp);
+
+	intel_dp_set_edid(intel_dp);
+
+	intel_dp_power_put(intel_dp, power_domain);
+
+	if (intel_encoder->type != INTEL_OUTPUT_EDP)
+		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+}
+
+static int intel_dp_get_modes(struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct edid *edid;
+
+	edid = intel_connector->detect_edid;
+	if (edid) {
+		int ret = intel_connector_update_modes(connector, edid);
+		if (ret)
+			return ret;
+	}
 
 	/* if eDP has no EDID, fall back to fixed mode */
-	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+	if (is_edp(intel_attached_dp(connector)) &&
+	    intel_connector->panel.fixed_mode) {
 		struct drm_display_mode *mode;
-		mode = drm_mode_duplicate(dev,
+
+		mode = drm_mode_duplicate(connector->dev,
 					  intel_connector->panel.fixed_mode);
 		if (mode) {
 			drm_mode_probed_add(connector, mode);
 			return 1;
 		}
 	}
+
 	return 0;
 }
 
 static bool
 intel_dp_detect_audio(struct drm_connector *connector)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-	if (edid) {
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid)
 		has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
 
 	return has_audio;
 }
@@ -4066,6 +4085,8 @@ intel_dp_connector_destroy(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
+	intel_dp_unset_edid(intel_attached_dp(connector));
+
 	if (!IS_ERR_OR_NULL(intel_connector->edid))
 		kfree(intel_connector->edid);
 
@@ -4118,6 +4139,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
+	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_set_property,
 	.destroy = intel_dp_connector_destroy,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 07217f39e925..f6a2b094384f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -214,6 +214,7 @@ struct intel_connector {
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
 	struct edid *edid;
+	struct edid *detect_edid;
 
 	/* since POLL and HPD connectors may use the same HPD line keep the native
 	   state of connector->polled in case hotplug storm detection changes it */
-- 
2.1.0

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

[PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

v2: Also hookup the force() callback for audio detection when the user
forces the connection status.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 145 +++++++++++++++++++++-----------------
 1 file changed, 80 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9169786dbbc3..7428521d2e25 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -962,104 +962,117 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
-static enum drm_connector_status
-intel_hdmi_detect(struct drm_connector *connector, bool force)
+static void
+intel_hdmi_unset_edid(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-	struct intel_digital_port *intel_dig_port =
-		hdmi_to_dig_port(intel_hdmi);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edid *edid;
-	enum intel_display_power_domain power_domain;
-	enum drm_connector_status status = connector_status_disconnected;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
+	intel_hdmi->has_hdmi_sink = false;
+	intel_hdmi->has_audio = false;
+	intel_hdmi->rgb_quant_range_selectable = false;
+
+	kfree(to_intel_connector(connector)->detect_edid);
+	to_intel_connector(connector)->detect_edid = NULL;
+}
+
+static bool
+intel_hdmi_set_edid(struct drm_connector *connector)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct intel_encoder *intel_encoder =
+		&hdmi_to_dig_port(intel_hdmi)->base;
+	enum intel_display_power_domain power_domain;
+	struct edid *edid;
+	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	intel_hdmi->has_hdmi_sink = false;
-	intel_hdmi->has_audio = false;
-	intel_hdmi->rgb_quant_range_selectable = false;
 	edid = drm_get_edid(connector,
 			    intel_gmbus_get_adapter(dev_priv,
 						    intel_hdmi->ddc_bus));
 
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
-			status = connector_status_connected;
-			if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
-				intel_hdmi->has_hdmi_sink =
-						drm_detect_hdmi_monitor(edid);
-			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
-			intel_hdmi->rgb_quant_range_selectable =
-				drm_rgb_quant_range_selectable(edid);
-		}
-		kfree(edid);
-	}
+	intel_display_power_put(dev_priv, power_domain);
+
+	to_intel_connector(connector)->detect_edid = edid;
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+		intel_hdmi->rgb_quant_range_selectable =
+			drm_rgb_quant_range_selectable(edid);
 
-	if (status == connector_status_connected) {
+		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 		if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
 			intel_hdmi->has_audio =
-				(intel_hdmi->force_audio == HDMI_AUDIO_ON);
-		intel_encoder->type = INTEL_OUTPUT_HDMI;
+				intel_hdmi->force_audio == HDMI_AUDIO_ON;
+
+		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
+			intel_hdmi->has_hdmi_sink =
+				drm_detect_hdmi_monitor(edid);
+
+		connected = true;
 	}
 
-	intel_display_power_put(dev_priv, power_domain);
+	return connected;
+}
+
+static enum drm_connector_status
+intel_hdmi_detect(struct drm_connector *connector, bool force)
+{
+	enum drm_connector_status status;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	intel_hdmi_unset_edid(connector);
+
+	if (intel_hdmi_set_edid(connector)) {
+		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+
+		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+		status = connector_status_connected;
+	} else
+		status = connector_status_disconnected;
 
 	return status;
 }
 
-static int intel_hdmi_get_modes(struct drm_connector *connector)
+static void
+intel_hdmi_force(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	int ret;
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 
-	/* We should parse the EDID data and find out if it's an HDMI sink so
-	 * we can send audio to it.
-	 */
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_hdmi_unset_edid(connector);
 
-	ret = intel_ddc_get_modes(connector,
-				   intel_gmbus_get_adapter(dev_priv,
-							   intel_hdmi->ddc_bus));
+	if (connector->status == connector_status_connected)
+		return;
 
-	intel_display_power_put(dev_priv, power_domain);
+	intel_hdmi_set_edid(connector);
+	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+}
 
-	return ret;
+static int intel_hdmi_get_modes(struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid == NULL)
+		return 0;
+
+	return intel_connector_update_modes(connector, edid);
 }
 
 static bool
 intel_hdmi_detect_audio(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL)
-			has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
+		has_audio = drm_detect_monitor_audio(edid);
 
 	return has_audio;
 }
@@ -1479,6 +1492,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 static void intel_hdmi_destroy(struct drm_connector *connector)
 {
+	intel_hdmi_unset_edid(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
@@ -1486,6 +1500,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_hdmi_detect,
+	.force = intel_hdmi_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_hdmi_set_property,
 	.destroy = intel_hdmi_destroy,
-- 
2.1.0

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

Re: [PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Ville Syrjälä]

On Tue, Sep 02, 2014 at 09:24:48AM +0100, Chris Wilson wrote:
> As we may query the edid multiple times following a detect, record the
> EDID found during output discovery and reuse it. This is a separate
> issue from caching the output EDID across detection cycles.
> 
> v2: Also hookup the force() callback for audio detection when the user
> forces the connection status.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 145 +++++++++++++++++++++-----------------
>  1 file changed, 80 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 9169786dbbc3..7428521d2e25 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -962,104 +962,117 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> -static enum drm_connector_status
> -intel_hdmi_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_hdmi_unset_edid(struct drm_connector *connector)
>  {
> -	struct drm_device *dev = connector->dev;
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> -	struct intel_digital_port *intel_dig_port =
> -		hdmi_to_dig_port(intel_hdmi);
> -	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct edid *edid;
> -	enum intel_display_power_domain power_domain;
> -	enum drm_connector_status status = connector_status_disconnected;
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> +	intel_hdmi->has_hdmi_sink = false;
> +	intel_hdmi->has_audio = false;
> +	intel_hdmi->rgb_quant_range_selectable = false;
> +
> +	kfree(to_intel_connector(connector)->detect_edid);
> +	to_intel_connector(connector)->detect_edid = NULL;
> +}
> +
> +static bool
> +intel_hdmi_set_edid(struct drm_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct intel_encoder *intel_encoder =
> +		&hdmi_to_dig_port(intel_hdmi)->base;
> +	enum intel_display_power_domain power_domain;
> +	struct edid *edid;
> +	bool connected = false;
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> -	intel_hdmi->has_hdmi_sink = false;
> -	intel_hdmi->has_audio = false;
> -	intel_hdmi->rgb_quant_range_selectable = false;
>  	edid = drm_get_edid(connector,
>  			    intel_gmbus_get_adapter(dev_priv,
>  						    intel_hdmi->ddc_bus));
>  
> -	if (edid) {
> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> -			status = connector_status_connected;
> -			if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
> -				intel_hdmi->has_hdmi_sink =
> -						drm_detect_hdmi_monitor(edid);
> -			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> -			intel_hdmi->rgb_quant_range_selectable =
> -				drm_rgb_quant_range_selectable(edid);
> -		}
> -		kfree(edid);
> -	}
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	to_intel_connector(connector)->detect_edid = edid;
> +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> +		intel_hdmi->rgb_quant_range_selectable =
> +			drm_rgb_quant_range_selectable(edid);
>  
> -	if (status == connector_status_connected) {
> +		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
>  		if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
>  			intel_hdmi->has_audio =
> -				(intel_hdmi->force_audio == HDMI_AUDIO_ON);
> -		intel_encoder->type = INTEL_OUTPUT_HDMI;
> +				intel_hdmi->force_audio == HDMI_AUDIO_ON;
> +
> +		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
> +			intel_hdmi->has_hdmi_sink =
> +				drm_detect_hdmi_monitor(edid);

I wonder if we should be updating has_hdmi_sink also in
intel_hdmi_set_property() when force_audio != HDMI_AUDIO_OFF_DVI.
But that's a separate issue so material for another patch.

> +
> +		connected = true;
>  	}
>  
> -	intel_display_power_put(dev_priv, power_domain);
> +	return connected;
> +}
> +
> +static enum drm_connector_status
> +intel_hdmi_detect(struct drm_connector *connector, bool force)
> +{
> +	enum drm_connector_status status;
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	intel_hdmi_unset_edid(connector);
> +
> +	if (intel_hdmi_set_edid(connector)) {
> +		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +
> +		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> +		status = connector_status_connected;
> +	} else
> +		status = connector_status_disconnected;
>  
>  	return status;
>  }
>  
> -static int intel_hdmi_get_modes(struct drm_connector *connector)
> +static void
> +intel_hdmi_force(struct drm_connector *connector)
>  {
> -	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
> -	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> -	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> -	enum intel_display_power_domain power_domain;
> -	int ret;
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  
> -	/* We should parse the EDID data and find out if it's an HDMI sink so
> -	 * we can send audio to it.
> -	 */
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
>  
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> +	intel_hdmi_unset_edid(connector);
>  
> -	ret = intel_ddc_get_modes(connector,
> -				   intel_gmbus_get_adapter(dev_priv,
> -							   intel_hdmi->ddc_bus));
> +	if (connector->status == connector_status_connected)

!=

Apart from that I didn't spot anything suspicious. So with this one
thing fixed you can slap on
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
for the series.

> +		return;
>  
> -	intel_display_power_put(dev_priv, power_domain);
> +	intel_hdmi_set_edid(connector);
> +	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> +}
>  
> -	return ret;
> +static int intel_hdmi_get_modes(struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +
> +	edid = to_intel_connector(connector)->detect_edid;
> +	if (edid == NULL)
> +		return 0;
> +
> +	return intel_connector_update_modes(connector, edid);
>  }
>  
>  static bool
>  intel_hdmi_detect_audio(struct drm_connector *connector)
>  {
> -	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
> -	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> -	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> -	enum intel_display_power_domain power_domain;
> -	struct edid *edid;
>  	bool has_audio = false;
> +	struct edid *edid;
>  
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> -
> -	edid = drm_get_edid(connector,
> -			    intel_gmbus_get_adapter(dev_priv,
> -						    intel_hdmi->ddc_bus));
> -	if (edid) {
> -		if (edid->input & DRM_EDID_INPUT_DIGITAL)
> -			has_audio = drm_detect_monitor_audio(edid);
> -		kfree(edid);
> -	}
> -
> -	intel_display_power_put(dev_priv, power_domain);
> +	edid = to_intel_connector(connector)->detect_edid;
> +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
> +		has_audio = drm_detect_monitor_audio(edid);
>  
>  	return has_audio;
>  }
> @@ -1479,6 +1492,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>  
>  static void intel_hdmi_destroy(struct drm_connector *connector)
>  {
> +	intel_hdmi_unset_edid(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> @@ -1486,6 +1500,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
>  	.dpms = intel_connector_dpms,
>  	.detect = intel_hdmi_detect,
> +	.force = intel_hdmi_force,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.set_property = intel_hdmi_set_property,
>  	.destroy = intel_hdmi_destroy,
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Jesse Barnes]

On Tue, 2 Sep 2014 13:45:37 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Tue, Sep 02, 2014 at 09:24:48AM +0100, Chris Wilson wrote:
> > As we may query the edid multiple times following a detect, record
> > the EDID found during output discovery and reuse it. This is a
> > separate issue from caching the output EDID across detection cycles.
> > 
> > v2: Also hookup the force() callback for audio detection when the
> > user forces the connection status.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 145
> > +++++++++++++++++++++----------------- 1 file changed, 80
> > insertions(+), 65 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c index
> > 9169786dbbc3..7428521d2e25 100644 ---
> > a/drivers/gpu/drm/i915/intel_hdmi.c +++
> > b/drivers/gpu/drm/i915/intel_hdmi.c @@ -962,104 +962,117 @@ bool
> > intel_hdmi_compute_config(struct intel_encoder *encoder, return
> > true; }
> >  
> > -static enum drm_connector_status
> > -intel_hdmi_detect(struct drm_connector *connector, bool force)
> > +static void
> > +intel_hdmi_unset_edid(struct drm_connector *connector)
> >  {
> > -	struct drm_device *dev = connector->dev;
> >  	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector);
> > -	struct intel_digital_port *intel_dig_port =
> > -		hdmi_to_dig_port(intel_hdmi);
> > -	struct intel_encoder *intel_encoder =
> > &intel_dig_port->base;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct edid *edid;
> > -	enum intel_display_power_domain power_domain;
> > -	enum drm_connector_status status =
> > connector_status_disconnected; 
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > -		      connector->base.id, connector->name);
> > +	intel_hdmi->has_hdmi_sink = false;
> > +	intel_hdmi->has_audio = false;
> > +	intel_hdmi->rgb_quant_range_selectable = false;
> > +
> > +	kfree(to_intel_connector(connector)->detect_edid);
> > +	to_intel_connector(connector)->detect_edid = NULL;
> > +}
> > +
> > +static bool
> > +intel_hdmi_set_edid(struct drm_connector *connector)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > to_i915(connector->dev);
> > +	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector);
> > +	struct intel_encoder *intel_encoder =
> > +		&hdmi_to_dig_port(intel_hdmi)->base;
> > +	enum intel_display_power_domain power_domain;
> > +	struct edid *edid;
> > +	bool connected = false;
> >  
> >  	power_domain =
> > intel_display_port_power_domain(intel_encoder);
> > intel_display_power_get(dev_priv, power_domain); 
> > -	intel_hdmi->has_hdmi_sink = false;
> > -	intel_hdmi->has_audio = false;
> > -	intel_hdmi->rgb_quant_range_selectable = false;
> >  	edid = drm_get_edid(connector,
> >  			    intel_gmbus_get_adapter(dev_priv,
> >  						    intel_hdmi->ddc_bus));
> >  
> > -	if (edid) {
> > -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> > -			status = connector_status_connected;
> > -			if (intel_hdmi->force_audio !=
> > HDMI_AUDIO_OFF_DVI)
> > -				intel_hdmi->has_hdmi_sink =
> > -
> > drm_detect_hdmi_monitor(edid);
> > -			intel_hdmi->has_audio =
> > drm_detect_monitor_audio(edid);
> > -			intel_hdmi->rgb_quant_range_selectable =
> > -
> > drm_rgb_quant_range_selectable(edid);
> > -		}
> > -		kfree(edid);
> > -	}
> > +	intel_display_power_put(dev_priv, power_domain);
> > +
> > +	to_intel_connector(connector)->detect_edid = edid;
> > +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> > +		intel_hdmi->rgb_quant_range_selectable =
> > +			drm_rgb_quant_range_selectable(edid);
> >  
> > -	if (status == connector_status_connected) {
> > +		intel_hdmi->has_audio =
> > drm_detect_monitor_audio(edid); if (intel_hdmi->force_audio !=
> > HDMI_AUDIO_AUTO) intel_hdmi->has_audio =
> > -				(intel_hdmi->force_audio ==
> > HDMI_AUDIO_ON);
> > -		intel_encoder->type = INTEL_OUTPUT_HDMI;
> > +				intel_hdmi->force_audio ==
> > HDMI_AUDIO_ON; +
> > +		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
> > +			intel_hdmi->has_hdmi_sink =
> > +				drm_detect_hdmi_monitor(edid);
> 
> I wonder if we should be updating has_hdmi_sink also in
> intel_hdmi_set_property() when force_audio != HDMI_AUDIO_OFF_DVI.
> But that's a separate issue so material for another patch.
> 
> > +
> > +		connected = true;
> >  	}
> >  
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	return connected;
> > +}
> > +
> > +static enum drm_connector_status
> > +intel_hdmi_detect(struct drm_connector *connector, bool force)
> > +{
> > +	enum drm_connector_status status;
> > +
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.id, connector->name);
> > +
> > +	intel_hdmi_unset_edid(connector);
> > +
> > +	if (intel_hdmi_set_edid(connector)) {
> > +		struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector); +
> > +		hdmi_to_dig_port(intel_hdmi)->base.type =
> > INTEL_OUTPUT_HDMI;
> > +		status = connector_status_connected;
> > +	} else
> > +		status = connector_status_disconnected;
> >  
> >  	return status;
> >  }
> >  
> > -static int intel_hdmi_get_modes(struct drm_connector *connector)
> > +static void
> > +intel_hdmi_force(struct drm_connector *connector)
> >  {
> > -	struct intel_encoder *intel_encoder =
> > intel_attached_encoder(connector);
> > -	struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(&intel_encoder->base);
> > -	struct drm_i915_private *dev_priv =
> > connector->dev->dev_private;
> > -	enum intel_display_power_domain power_domain;
> > -	int ret;
> > +	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector); 
> > -	/* We should parse the EDID data and find out if it's an
> > HDMI sink so
> > -	 * we can send audio to it.
> > -	 */
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.id, connector->name);
> >  
> > -	power_domain =
> > intel_display_port_power_domain(intel_encoder);
> > -	intel_display_power_get(dev_priv, power_domain);
> > +	intel_hdmi_unset_edid(connector);
> >  
> > -	ret = intel_ddc_get_modes(connector,
> > -
> > intel_gmbus_get_adapter(dev_priv,
> > -
> > intel_hdmi->ddc_bus));
> > +	if (connector->status == connector_status_connected)
> 
> !=
> 
> Apart from that I didn't spot anything suspicious. So with this one
> thing fixed you can slap on
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> for the series.
> 
> > +		return;
> >  
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	intel_hdmi_set_edid(connector);
> > +	hdmi_to_dig_port(intel_hdmi)->base.type =
> > INTEL_OUTPUT_HDMI; +}
> >  
> > -	return ret;
> > +static int intel_hdmi_get_modes(struct drm_connector *connector)
> > +{
> > +	struct edid *edid;
> > +
> > +	edid = to_intel_connector(connector)->detect_edid;
> > +	if (edid == NULL)
> > +		return 0;
> > +
> > +	return intel_connector_update_modes(connector, edid);
> >  }
> >  
> >  static bool
> >  intel_hdmi_detect_audio(struct drm_connector *connector)
> >  {
> > -	struct intel_encoder *intel_encoder =
> > intel_attached_encoder(connector);
> > -	struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(&intel_encoder->base);
> > -	struct drm_i915_private *dev_priv =
> > connector->dev->dev_private;
> > -	enum intel_display_power_domain power_domain;
> > -	struct edid *edid;
> >  	bool has_audio = false;
> > +	struct edid *edid;
> >  
> > -	power_domain =
> > intel_display_port_power_domain(intel_encoder);
> > -	intel_display_power_get(dev_priv, power_domain);
> > -
> > -	edid = drm_get_edid(connector,
> > -			    intel_gmbus_get_adapter(dev_priv,
> > -
> > intel_hdmi->ddc_bus));
> > -	if (edid) {
> > -		if (edid->input & DRM_EDID_INPUT_DIGITAL)
> > -			has_audio = drm_detect_monitor_audio(edid);
> > -		kfree(edid);
> > -	}
> > -
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	edid = to_intel_connector(connector)->detect_edid;
> > +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
> > +		has_audio = drm_detect_monitor_audio(edid);
> >  
> >  	return has_audio;
> >  }
> > @@ -1479,6 +1492,7 @@ static void chv_hdmi_pre_enable(struct
> > intel_encoder *encoder) 
> >  static void intel_hdmi_destroy(struct drm_connector *connector)
> >  {
> > +	intel_hdmi_unset_edid(connector);
> >  	drm_connector_cleanup(connector);
> >  	kfree(connector);
> >  }
> > @@ -1486,6 +1500,7 @@ static void intel_hdmi_destroy(struct
> > drm_connector *connector) static const struct drm_connector_funcs
> > intel_hdmi_connector_funcs = { .dpms = intel_connector_dpms,
> >  	.detect = intel_hdmi_detect,
> > +	.force = intel_hdmi_force,
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> >  	.set_property = intel_hdmi_set_property,
> >  	.destroy = intel_hdmi_destroy,
> > -- 
> > 2.1.0
> 


Tried to find a JIRA for this but only came up with VIZ-1789.  Looks
like it might be close enough for Wayland?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Daniel Vetter]

On Thu, Sep 11, 2014 at 11:20:13AM -0700, Jesse Barnes wrote:
> Tried to find a JIRA for this but only came up with VIZ-1789.  Looks
> like it might be close enough for Wayland?

Different thing - this only avoids reprobing within a single detect cycle
when we grab the edid multiple times. Edid caching is caching between
different callers so that wayland doesn't block when the kernel already
grabbed the edid.

Or maybe it's too early for me again and I'm confused ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Chris Wilson]

On Fri, Sep 12, 2014 at 10:34:23AM +0200, Daniel Vetter wrote:
> On Thu, Sep 11, 2014 at 11:20:13AM -0700, Jesse Barnes wrote:
> > Tried to find a JIRA for this but only came up with VIZ-1789.  Looks
> > like it might be close enough for Wayland?
> 
> Different thing - this only avoids reprobing within a single detect cycle
> when we grab the edid multiple times. Edid caching is caching between
> different callers so that wayland doesn't block when the kernel already
> grabbed the edid.
> 
> Or maybe it's too early for me again and I'm confused ;-)

Should be different. Or maybe the question is why is wayland doing more
than one probe per hotplug pulse?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre