Re: [PATCH] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2015-11-17 at 11:56 +0530, Shubhangi Shrivastava wrote:
> This patch moves probing for panel, DPCD read etc to another
> function intel_dp_long_pulse, while intel_dp_detect returns
> the status as connected or disconnected depending on
> whether the edid is available or not.

May i suggest a slight rephrase?

"This patch moves probing for panel, DPCD read, etc done in intel_dp_detect() to
a new function: intel_dp_long_pulse(). Note that the behavior of
intel_dp_detect() is changed to report connected or disconnected depending on
whether the edid is available or not."

I think we also need a note why the EDID change is necessary and at the same
doesn't break anything.


> This change will be required by further patches in the series
> to avoid performing multiple DPCD operations and removing
> their duplication.

I found this last part confusing. Maybe replace "avoid multiple DPCD operations
and removing their duplication" with "avoid duplicated DPCD operations on
hotplug", since we are not going into details here.

> 
> v2: Moved a hunk to next patch of the series.
>     Moved intel_dp_unset_edid to out. (Ander)
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++------------
> -
>  1 file changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8071247..dd2b9da 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4631,9 +4631,10 @@ intel_dp_power_put(struct intel_dp *dp,
>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>  }
>  
> -static enum drm_connector_status
> -intel_dp_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> +	struct drm_connector *connector = &intel_connector->base;
>  	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;
> @@ -4643,17 +4644,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	bool ret;
>  	u8 sink_irq_vector;
>  
> -	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;
> -		return connector_status_disconnected;
> -	}
> -
>  	power_domain = intel_dp_power_get(intel_dp);
>  
>  	/* Can't disconnect eDP, but you can close the lid... */
> @@ -4670,8 +4660,10 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  
>  	ret = intel_dp_probe_mst(intel_dp);
>  	if (ret) {
> -		/* if we are in MST mode then this connector
> -		   won't appear connected or have anything with EDID on it */
> +		/*
> +		 * if we are in MST mode then this connector
> +		 * won't appear connected or have anything with EDID on it
> +		 */
>  		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
> @@ -4699,8 +4691,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	}
>  
>  out:
> +	if (status != connector_status_connected)
> +		intel_dp_unset_edid(intel_dp);

I didn't notice this at first, but intel_dp_set_edid() doesn't free the previous
edid, so I think we also need to call it just before intel_dp_set_edid(). Patch
looks good otherwise.

Ander

>  	intel_dp_power_put(intel_dp, power_domain);
> -	return status;
> +	return;
> +}
> +
> +static enum drm_connector_status
> +intel_dp_detect(struct drm_connector *connector, bool force)
> +{
> +	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);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	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;
> +		return connector_status_disconnected;
> +	}
> +
> +	intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	if (intel_connector->detect_edid)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
>  }
>  
>  static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx