Re: [PATCH] drm/i915: Cleaning up intel_dp_hpd_pulse

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

On Tue, 2015-11-17 at 12:17 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (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 | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dd2b9da..e4d6d33 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4668,6 +4668,16 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
>  		goto out;
> +	} else if (connector->status == connector_status_connected) {
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		goto out;
>  	}
>  
>  	intel_dp_set_edid(intel_dp);
> @@ -4715,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5045,21 +5056,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		goto put_power;

This has the problem with handling an mst device that is gone which Sivakumar
and I discussed on IRC.

I'm thinking whether it makes sense to move the code in the mst_fail label to 
intel_dp_detect(), since it gets called every time we reach that label. We would
just need a way to tell it about the failure, since we can't rely on connector
status being disconnected (because of MST). That could help us clean up
intel_dp_hpd_pulse even more.
Thoughts?

Ander

>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx