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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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