Re: [PATCH 06/12] drm/i915: Allow MST sinks to work even if drm_probe_ddc() fails

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 28, 2016 at 05:50:42PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> With HSW + Dell UP2414Q (at least) drm_probe_ddc() occasionally fails,
> and then we'll assume that the entire display has been disconnected.
> We don't need the EDID from the main link, so we can simply check if
> the sink is MST capable, and if so treat is as connected.
> 
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Manasi D Navare <manasi.d.navare@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 45 +++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3a9c5d3b5c66..4a4184c21989 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3538,7 +3538,7 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  }
>  
>  static bool
> -intel_dp_probe_mst(struct intel_dp *intel_dp)
> +intel_dp_can_mst(struct intel_dp *intel_dp)
>  {
>  	u8 buf[1];
>  
> @@ -3551,18 +3551,30 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
>  		return false;
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> -		if (buf[0] & DP_MST_CAP) {
> -			DRM_DEBUG_KMS("Sink is MST capable\n");
> -			intel_dp->is_mst = true;
> -		} else {
> -			DRM_DEBUG_KMS("Sink is not MST capable\n");
> -			intel_dp->is_mst = false;
> -		}
> -	}
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1) != 1)
> +		return false;
>  
> -	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> -	return intel_dp->is_mst;
> +	return buf[0] & DP_MST_CAP;
> +}
> +
> +static void
> +intel_dp_configure_mst(struct intel_dp *intel_dp)
> +{
> +	if (!i915.enable_dp_mst)
> +		return;
> +
> +	if (!intel_dp->can_mst)
> +		return;
> +
> +	intel_dp->is_mst = intel_dp_can_mst(intel_dp);

can_mst (is the hw capable) vs. can_mst (is the sink capable). Needs a
can_sink_mst or something else.

Also this really should be part of the mst helpers imo ...

> +
> +	if (intel_dp->is_mst)
> +		DRM_DEBUG_KMS("Sink is MST capable\n");
> +	else
> +		DRM_DEBUG_KMS("Sink is not MST capable\n");
> +
> +	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +					intel_dp->is_mst);
>  }
>  
>  static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> @@ -3993,6 +4005,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
>  
> +	if (intel_dp_can_mst(intel_dp))
> +		return connector_status_connected;

Shouldn't we instead just outright not poke the ddc when there's an mst
branch connected? The dp mst helpers will read the ddc for the final leaf
ports, anything intermediate is kinda bonghits anyway. So

  	if (!intel_dp_can_mst() && drm_probe_ddc(&intel_dp->aux.ddc))
  		return connector_status_connected;

I think with that it makes a lot more sense and is

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

And again, this so should be all shared in dp helpers somehow.
-Daniel

> +
>  	/* Well we tried, say unknown for unreliable port types */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
>  		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> @@ -4213,7 +4228,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	struct drm_device *dev = connector->dev;
>  	enum drm_connector_status status;
>  	enum intel_display_power_domain power_domain;
> -	bool ret;
>  	u8 sink_irq_vector;
>  
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
> @@ -4249,8 +4263,9 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  
>  	intel_dp_probe_oui(intel_dp);
>  
> -	ret = intel_dp_probe_mst(intel_dp);
> -	if (ret) {
> +	intel_dp_configure_mst(intel_dp);
> +
> +	if (intel_dp->is_mst) {
>  		/*
>  		 * If we are in MST mode then this connector
>  		 * won't appear connected or have anything
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx