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

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

On Fri, Jul 29, 2016 at 11:29:56AM +0200, Daniel Vetter wrote:
> 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?

I suppose. We won't read the EDID anyway. Not really sure why this
display has problems with DDC sometimes, but then again it has a lot of
other problems too, the biggest of which is that it refuses to do the
payload allocation if I reboot the machine with display already on. I
have to turn it off and on again after rebooting to get it back into a
working state.

> 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.

Yeah, we should try to unify all of the probe and long/short pulse
handling across drivers.

> -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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx