Re: [PATCH] drm/i915: Improve DP downstream HPD handling

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

On Wed, Jul 08, 2015 at 05:50:05PM +0530, Sivakumar Thulasimani wrote:
> 
> 
> On 7/7/2015 5:50 PM, Sivakumar Thulasimani wrote:
> >
> >
> > On 7/7/2015 5:24 PM, Ville Syrjälä wrote:
> >> On Tue, Jul 07, 2015 at 02:37:46PM +0300, Ville Syrjälä wrote:
> >>> On Tue, Jul 07, 2015 at 04:45:11PM +0530, Sivakumar Thulasimani wrote:
> >>>>
> >>>> On 7/7/2015 4:40 PM, Ville Syrjälä wrote:
> >>>>> On Tue, Jul 07, 2015 at 03:26:36PM +0530, Sivakumar Thulasimani 
> >>>>> wrote:
> >>>>>> On 7/6/2015 5:42 PM, ville.syrjala@linux.intel.com wrote:
> >>>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>>>
> >>>>>>> DP dongles may signal downstream HPD via short HPD pulses. If we 
> >>>>>>> know
> >>>>>>> the device has a HPD capable downstream port, make sure we kick 
> >>>>>>> off the
> >>>>>>> full hotplug processing even for short HPDs.
> >>>>>>>
> >>>>>>> Additonally setting the sink to DPMS off kills the downstream 
> >>>>>>> HPD (at
> >>>>>>> least on my DP->VGA dongle), so skip the DPMS off for such dongles
> >>>>>>> when we turn off the port.
> >>>>>>>
> >>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>>> ---
> >>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 18 +++++++++++++++++-
> >>>>>>>     1 file changed, 17 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> >>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>> index e88cec2..f424833 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>> @@ -2324,6 +2324,13 @@ static void intel_dp_get_config(struct 
> >>>>>>> intel_encoder *encoder,
> >>>>>>>         }
> >>>>>>>     }
> >>>>>>>     +static bool intel_dp_has_downstream_hpd(struct intel_dp 
> >>>>>>> *intel_dp)
> >>>>>>> +{
> >>>>>>> +    return intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & 
> >>>>>>> DP_DWN_STRM_PORT_PRESENT &&
> >>>>>>> +        intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> >>>>>>> +        intel_dp->downstream_ports[0] & DP_DS_PORT_HPD;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>     static void intel_disable_dp(struct intel_encoder *encoder)
> >>>>>>>     {
> >>>>>>>         struct intel_dp *intel_dp = 
> >>>>>>> enc_to_intel_dp(&encoder->base);
> >>>>>>> @@ -2340,7 +2347,9 @@ static void intel_disable_dp(struct 
> >>>>>>> intel_encoder *encoder)
> >>>>>>>          * ensure that we have vdd while we switch off the 
> >>>>>>> panel. */
> >>>>>>>         intel_edp_panel_vdd_on(intel_dp);
> >>>>>>>         intel_edp_backlight_off(intel_dp);
> >>>>>>> -    intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>>> +    /* Skip power down to keep downstream HPD working */
> >>>>>>> +    if (!intel_dp_has_downstream_hpd(intel_dp))
> >>>>>>> +        intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>>>>>>         intel_edp_panel_off(intel_dp);
> >>>>>>>             /* disable the port before the pipe on g4x */
> >>>>>>> @@ -4944,6 +4953,13 @@ intel_dp_hpd_pulse(struct 
> >>>>>>> intel_digital_port *intel_dig_port, bool long_hpd)
> >>>>>>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>>>>>>                 intel_dp_check_link_status(intel_dp);
> >>>>>>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >>>>>>> +
> >>>>>>> +            /*
> >>>>>>> +             * Downstream HPD will generate a short HPD,
> >>>>>>> +             * so we want full hotplug processing here.
> >>>>>>> +             */
> >>>>>>> +            if (intel_dp_has_downstream_hpd(intel_dp))
> >>>>>>> +                goto put_power;
> >>>>>>>             }
> >>>>>>>         }
> >>>>>> I am looking into compliance changes for DP and this seems a 
> >>>>>> relevant
> >>>>>> change for compliance as well. but as per Link CTS 1.2 section 
> >>>>>> 4.2.2.8,
> >>>>>> we are supposed to read the sink_count and do full detection if
> >>>>>> sink_count is >1.  So instead of checking for DP_DS_PORT_HPD can 
> >>>>>> we just
> >>>>>> check SINK_COUNT and do full detect ?
> >>>>> ->detect() will be called from the hotplug work and that will
> >>>>> check SINK_COUNT.
> >>>>>
> >>>> No, the Compliance Sink tool, will not set the DP_DS_PORT_HPD 
> >>>> resulting
> >>>> in detect not getting executed for
> >>>> the short pulse generated. The spec requires the sink to set only the
> >>>> sink count so it is not a must for
> >>>> the sink to update the DP_DOWNSTREAM_PORT_0. so only a check for
> >>>> SINK_COUNT will pass the
> >>>> compliance test.
> >>> That seems stupid. If the downstream port isn't HPD capable then we 
> >>> have
> >>> no reason to check SINK_COUNT after a short HPD as the short HPD
> >>> coudln't have been caused by a downstram HPD. Obviuously we still
> >>> check SINK_COUNT after a long HPD to figure out if anything is 
> >>> connected
> >>> when the branch device itself gets connected to the source.
> >> Actually that's not correct. We don't check SINK_COUNT unless the 
> >> downstream
> >> port is HPD capable.
> >>
> >> The spec says:
> >> "If the DFP does not provide for means for plug/unplug detection, the
> >> adaptor must set the SINK_COUNT field bits, as if those Sink devices 
> >> were
> >> all permanently plugged."
> >>
> >> So according to the there can't be any changes in SINK_COUNT if the
> >> downstream port is not HPD capable.
> >>
> >>
> >>
> > yes, agree on the no changes for SINK_COUNT if HPD is 0. i'll check 
> > with DP Compliance test
> > tomorrow and confirm the exact reason for its failure may be my 
> > understanding of it was incorrect.
> >
> confirmed that the compliance sink is not setting HPD bit during detect. 
> so this looks to be a bug in
> the sink tool. i'll file a bug with their team instead.
> 
> coming back to this patch, i will get back once i understand the complex 
> scenario of all short pulse
> is treated as long pulse post this change, for example: we will do full 
> detection even if the sink requested
> retraining of link.

I suppose we should read the reason for the short HPD from some DPCD
register, assuming one exists. I've not trawled the spec for such a
thing so can't say for sure.

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