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@xxxxxxxxxxxxxxx wrote:
> >>>>>>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>>>>>>
> >>>>>>> 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@xxxxxxxxxxxxxxx>
> >>>>>>> ---
> >>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
