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

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

[ville.syrjala]

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;
 		}
 	}
 
-- 
2.3.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sivakumar Thulasimani]

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 ?

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

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

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

[Sivakumar Thulasimani]

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.

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

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

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

[Ville Syrjälä]

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.

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

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

[Sivakumar Thulasimani]

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.


-- 
regards,
Sivakumar


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sivakumar Thulasimani]

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.

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

DP dongles may signal downstream HPD via short HPD pulses. Setting the
sink to DPMS off apparently 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.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Pablo <pablodebiase@nanalysis.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103472
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99114
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 aa75f55eeb61..6ecc1b532f15 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2673,6 +2673,21 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 	}
 }
 
+static bool downstream_hpd_needs_d0(struct intel_dp *intel_dp)
+{
+	/*
+	 * DPCD 1.2 should support BRANCH_DEVICE_CTRL, and thus
+	 * be capable of signalling downstream hpd with a long pulse.
+	 * Whether or not that means D3 is safe to use is not clear,
+	 * but let's assume so until proven otherwise.
+	 *
+	 * FIXME should really check all downstream ports...
+	 */
+	return intel_dp->dpcd[DP_DPCD_REV] == 0x11 &&
+		intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT &&
+		intel_dp->downstream_ports[0] & DP_DS_PORT_HPD;
+}
+
 static void intel_disable_dp(struct intel_encoder *encoder,
 			     const struct intel_crtc_state *old_crtc_state,
 			     const struct drm_connector_state *old_conn_state)
@@ -2686,7 +2701,8 @@ 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(old_conn_state);
-	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
+	if (!downstream_hpd_needs_d0(intel_dp))
+		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	intel_edp_panel_off(intel_dp);
 }
 
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2017-10-26 at 22:41 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP dongles may signal downstream HPD via short HPD pulses. Setting the
> sink to DPMS off apparently 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.
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Pablo <pablodebiase@nanalysis.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103472
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99114
> 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 aa75f55eeb61..6ecc1b532f15 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2673,6 +2673,21 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  	}
>  }
>  
> +static bool downstream_hpd_needs_d0(struct intel_dp *intel_dp)
> +{
> +	/*
> +	 * DPCD 1.2 should support BRANCH_DEVICE_CTRL, and thus
> +	 * be capable of signalling downstream hpd with a long pulse.
> +	 * Whether or not that means D3 is safe to use is not clear,
> +	 * but let's assume so until proven otherwise.
> +	 *
> +	 * FIXME should really check all downstream ports...
> +	 */
> +	return intel_dp->dpcd[DP_DPCD_REV] == 0x11 &&
> +		intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT &&
> +		intel_dp->downstream_ports[0] & DP_DS_PORT_HPD;
> +}
> +
>  static void intel_disable_dp(struct intel_encoder *encoder,
>  			     const struct intel_crtc_state *old_crtc_state,
>  			     const struct drm_connector_state *old_conn_state)
> @@ -2686,7 +2701,8 @@ static void intel_disable_dp(struct intel_encoder *encoder,

It's not clear why the DDI counterpart is missing.

>  	 * ensure that we have vdd while we switch off the panel. */
>  	intel_edp_panel_vdd_on(intel_dp);
>  	intel_edp_backlight_off(old_conn_state);
> -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> +	if (!downstream_hpd_needs_d0(intel_dp))
> +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>  	intel_edp_panel_off(intel_dp);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Oct 27, 2017 at 12:48:39AM +0000, Pandiyan, Dhinakaran wrote:
> 
> On Thu, 2017-10-26 at 22:41 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DP dongles may signal downstream HPD via short HPD pulses. Setting the
> > sink to DPMS off apparently 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.
> > 
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Pablo <pablodebiase@nanalysis.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103472
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99114
> > 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 aa75f55eeb61..6ecc1b532f15 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2673,6 +2673,21 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
> >  	}
> >  }
> >  
> > +static bool downstream_hpd_needs_d0(struct intel_dp *intel_dp)
> > +{
> > +	/*
> > +	 * DPCD 1.2 should support BRANCH_DEVICE_CTRL, and thus
> > +	 * be capable of signalling downstream hpd with a long pulse.
> > +	 * Whether or not that means D3 is safe to use is not clear,
> > +	 * but let's assume so until proven otherwise.
> > +	 *
> > +	 * FIXME should really check all downstream ports...
> > +	 */
> > +	return intel_dp->dpcd[DP_DPCD_REV] == 0x11 &&
> > +		intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT &&
> > +		intel_dp->downstream_ports[0] & DP_DS_PORT_HPD;
> > +}
> > +
> >  static void intel_disable_dp(struct intel_encoder *encoder,
> >  			     const struct intel_crtc_state *old_crtc_state,
> >  			     const struct drm_connector_state *old_conn_state)
> > @@ -2686,7 +2701,8 @@ static void intel_disable_dp(struct intel_encoder *encoder,
> 
> It's not clear why the DDI counterpart is missing.

Because I forgot we have the same code in two places ;) v2 coming up...

> 
> >  	 * ensure that we have vdd while we switch off the panel. */
> >  	intel_edp_panel_vdd_on(intel_dp);
> >  	intel_edp_backlight_off(old_conn_state);
> > -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > +	if (!downstream_hpd_needs_d0(intel_dp))
> > +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  	intel_edp_panel_off(intel_dp);
> >  }
> >  

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