| Message ID | 1436184768-18920-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ?
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.
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.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6728
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -1 302/302 301/302
SNB 312/316 312/316
IVB 345/345 345/345
BYT -1 289/289 288/289
HSW 382/382 382/382
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-rmfb-interruptible PASS(1) DMESG_WARN(1)
(dmesg patch applied)drm:intel_pch_fifo_underrun_irq_handler[i915]]*ERROR*PCH_transcoder_A_FIFO_underrun@PCH transcoder A FIFO underrun
*BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
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.
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.
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.
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.
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.
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; } }