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@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > Cc: Manasi D Navare <manasi.d.navare@xxxxxxxxx>
> > Cc: Durgadoss R <durgadoss.r@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
