[PATCH 0/3] Detect DP displays based on sink count change

[PATCH 0/3] Detect DP displays based on sink count change

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

These patches together help detect DP displays on short pulse HPD 
and pass the respective compliance test case (4.2.2.8)

v2: modifed first patch so we will read sink_count independent of
downstream ports availablility.

Thulasimani,Sivakumar (3):
  drm/i915: read sink_count dpcd always
  drm/i915: Save sink_count for tracking changes to it
  drm/i915: force full detect on sink count change

 drivers/gpu/drm/i915/intel_dp.c  |   50 +++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 29 insertions(+), 22 deletions(-)

-- 
1.7.9.5

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

[PATCH 1/3] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

This patch reads sink_count dpcd always and removes its
read operation based on values in downstream port dpcd. Also
we should read it irrespective of current status of sink.

SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
SINK_COUNT denotes if a display is attached, while
DOWNSTREAM_PORT_PRESET indicates how many ports are available
in the dongle where display can be attached. so it is possible
for sink count to change irrespective of value in downstream
port dpcd.

Here is a table of possible values and scenarios

sink_count	downstream_port
		present
0		0		no display is attached
0		1		dongle is connected without display
1		0		display connected directly
1		1		display connected through dongle

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8a66a44..9e4e27d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
+	uint8_t reg;
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
 				    sizeof(intel_dp->dpcd)) < 0)
@@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+					    &reg, 1) < 0)
+			return false;
+
+	if (!DP_GET_SINK_COUNT(reg))
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
@@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
@@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
 		return connector_status_connected;
 
-	/* If we're HPD-aware, SINK_COUNT changes dynamically */
-	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
-	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
-		uint8_t reg;
-
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
-			return connector_status_unknown;
-
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
-	}
-
 	/* If no HPD, poke DDC gently */
 	if (drm_probe_ddc(&intel_dp->aux.ddc))
 		return connector_status_connected;
-- 
1.7.9.5

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

[PATCH 2/3] drm/i915: Save sink_count for tracking changes to it

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |    5 ++---
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9e4e27d..834f513 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3898,7 +3898,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
-	uint8_t reg;
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
 				    sizeof(intel_dp->dpcd)) < 0)
@@ -3910,10 +3909,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		return false; /* DPCD not present */
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
+					    &intel_dp->sink_count, 1) < 0)
 			return false;
 
-	if (!DP_GET_SINK_COUNT(reg))
+	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
 		return false;
 
 	/* Check if the panel supports PSR */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..8aca5bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_dp {
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
+	uint8_t sink_count;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
-- 
1.7.9.5

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

[PATCH 3/3] drm/i915: force full detect on sink count change

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

This patch checks for changes in sink count between short pulse
hpds and forces full detect when there is a change. This will
result in compliance test case 4.2.2.8 passing since it tests
for this behavior.

This will allow both detection of hotplug and unplug of panels
through dongles that give only short pulse for such events.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 834f513..eb3ab41 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4370,26 +4370,34 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	u8 sink_irq_vector;
+	u8 old_sink_count = intel_dp->sink_count;
+	u8 ret;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
+	*perform_full_detect = false;
+
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
 	}
 
 	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
+	ret = intel_dp_get_dpcd(intel_dp);
+
+	if (old_sink_count != intel_dp->sink_count)
+		*perform_full_detect = true;
+
+	if (!ret)
 		return;
-	}
 
 	if (!intel_encoder->base.crtc)
 		return;
@@ -5031,13 +5039,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
+			bool full_detect;
 			/*
 			 * we'll check the link status via the normal hot plug path later -
 			 * but for short hpds we should check it now
 			 */
 			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
+			intel_dp_check_link_status(intel_dp, &full_detect);
 			drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+			if (full_detect)
+				goto put_power;
 		}
 	}
 
-- 
1.7.9.5

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

Re: [PATCH 1/3] drm/i915: read sink_count dpcd always

[Jani Nikula]

On Tue, 25 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> This patch reads sink_count dpcd always and removes its
> read operation based on values in downstream port dpcd. Also
> we should read it irrespective of current status of sink.

Having to write "also" in a commit message is a good hint to stop and
think about whether that should be a separate patch. Maybe it should be,
here too?

BR,
Jani.

>
> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
> SINK_COUNT denotes if a display is attached, while
> DOWNSTREAM_PORT_PRESET indicates how many ports are available
> in the dongle where display can be attached. so it is possible
> for sink count to change irrespective of value in downstream
> port dpcd.
>
> Here is a table of possible values and scenarios
>
> sink_count	downstream_port
> 		present
> 0		0		no display is attached
> 0		1		dongle is connected without display
> 1		0		display connected directly
> 1		1		display connected through dongle
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8a66a44..9e4e27d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
> +	uint8_t reg;
>  
>  	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>  				    sizeof(intel_dp->dpcd)) < 0)
> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>  		return false; /* DPCD not present */
>  
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +					    &reg, 1) < 0)
> +			return false;
> +
> +	if (!DP_GET_SINK_COUNT(reg))
> +		return false;
> +
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> @@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> -
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> -
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;
> @@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;
> +
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>  		return connector_status_connected;
>  
> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> -		uint8_t reg;
> -
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &reg, 1) < 0)
> -			return connector_status_unknown;
> -
> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> -					      : connector_status_disconnected;
> -	}
> -
>  	/* If no HPD, poke DDC gently */
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: force full detect on sink count change

[Jani Nikula]

On Tue, 25 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> This patch checks for changes in sink count between short pulse
> hpds and forces full detect when there is a change. This will
> result in compliance test case 4.2.2.8 passing since it tests
> for this behavior.
>
> This will allow both detection of hotplug and unplug of panels
> through dongles that give only short pulse for such events.
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 834f513..eb3ab41 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4370,26 +4370,34 @@ go_again:
>   *  4. Check link status on receipt of hot-plug interrupt
>   */
>  static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)

You could turn perform_full_detect into a boolean return value which
this function doesn't have.

>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>  	u8 sink_irq_vector;
> +	u8 old_sink_count = intel_dp->sink_count;
> +	u8 ret;

Isn't this a bool, really?

>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> +	*perform_full_detect = false;
> +
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;
>  	}
>  
>  	/* Now read the DPCD to see if it's actually running */
> -	if (!intel_dp_get_dpcd(intel_dp)) {
> +	ret = intel_dp_get_dpcd(intel_dp);
> +
> +	if (old_sink_count != intel_dp->sink_count)
> +		*perform_full_detect = true;

Can't trust intel_dp->sink_count without checking the return value. So I
guess you can call intel_dp_get_dpcd the way it's done now, and move the
sink count check below it.

BR,
Jani.

> +
> +	if (!ret)
>  		return;
> -	}
>  
>  	if (!intel_encoder->base.crtc)
>  		return;
> @@ -5031,13 +5039,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		}
>  
>  		if (!intel_dp->is_mst) {
> +			bool full_detect;
>  			/*
>  			 * we'll check the link status via the normal hot plug path later -
>  			 * but for short hpds we should check it now
>  			 */
>  			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> +			intel_dp_check_link_status(intel_dp, &full_detect);
>  			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +			if (full_detect)
> +				goto put_power;
>  		}
>  	}
>  
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

On 8/26/2015 3:17 PM, Jani Nikula wrote:
> On Tue, 25 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> This patch reads sink_count dpcd always and removes its
>> read operation based on values in downstream port dpcd. Also
>> we should read it irrespective of current status of sink.
> Having to write "also" in a commit message is a good hint to stop and
> think about whether that should be a separate patch. Maybe it should be,
> here too?
>
> BR,
> Jani.
:) i thought about it since that part of the message was added last, since
without those changes this patch will not be "read sink_count always".

will split it into two patches and rename them accordingly.
>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>> SINK_COUNT denotes if a display is attached, while
>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>> in the dongle where display can be attached. so it is possible
>> for sink count to change irrespective of value in downstream
>> port dpcd.
>>
>> Here is a table of possible values and scenarios
>>
>> sink_count	downstream_port
>> 		present
>> 0		0		no display is attached
>> 0		1		dongle is connected without display
>> 1		0		display connected directly
>> 1		1		display connected through dongle
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
>>   1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8a66a44..9e4e27d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	struct drm_device *dev = dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	uint8_t rev;
>> +	uint8_t reg;
>>   
>>   	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>   				    sizeof(intel_dp->dpcd)) < 0)
>> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>   		return false; /* DPCD not present */
>>   
>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +					    &reg, 1) < 0)
>> +			return false;
>> +
>> +	if (!DP_GET_SINK_COUNT(reg))
>> +		return false;
>> +
>>   	/* Check if the panel supports PSR */
>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>   	if (is_edp(intel_dp)) {
>> @@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> -
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> -
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
>> @@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   		return;
>>   	}
>>   
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
>> +
>>   	/* Try to read the source of the interrupt */
>>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>   	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>   		return connector_status_connected;
>>   
>> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>> -		uint8_t reg;
>> -
>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> -					    &reg, 1) < 0)
>> -			return connector_status_unknown;
>> -
>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> -					      : connector_status_disconnected;
>> -	}
>> -
>>   	/* If no HPD, poke DDC gently */
>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>   		return connector_status_connected;
>> -- 
>> 1.7.9.5
>>

-- 
regards,
Sivakumar

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

Re: [PATCH 3/3] drm/i915: force full detect on sink count change

[Sivakumar Thulasimani]

On 8/26/2015 3:32 PM, Jani Nikula wrote:
> On Tue, 25 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> This patch checks for changes in sink count between short pulse
>> hpds and forces full detect when there is a change. This will
>> result in compliance test case 4.2.2.8 passing since it tests
>> for this behavior.
>>
>> This will allow both detection of hotplug and unplug of panels
>> through dongles that give only short pulse for such events.
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 834f513..eb3ab41 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4370,26 +4370,34 @@ go_again:
>>    *  4. Check link status on receipt of hot-plug interrupt
>>    */
>>   static void
>> -intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
> You could turn perform_full_detect into a boolean return value which
> this function doesn't have.
sure will do.
>
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>   	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	struct intel_crtc *crtc =
>>   		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>>   	u8 sink_irq_vector;
>> +	u8 old_sink_count = intel_dp->sink_count;
>> +	u8 ret;
> Isn't this a bool, really?
>
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> +	*perform_full_detect = false;
>> +
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
>>   	}
>>   
>>   	/* Now read the DPCD to see if it's actually running */
>> -	if (!intel_dp_get_dpcd(intel_dp)) {
>> +	ret = intel_dp_get_dpcd(intel_dp);
>> +
>> +	if (old_sink_count != intel_dp->sink_count)
>> +		*perform_full_detect = true;
> Can't trust intel_dp->sink_count without checking the return value. So I
> guess you can call intel_dp_get_dpcd the way it's done now, and move the
> sink count check below it.
>
> BR,
> Jani.

sure will fix this.
>
>> +
>> +	if (!ret)
>>   		return;
>> -	}
>>   
>>   	if (!intel_encoder->base.crtc)
>>   		return;
>> @@ -5031,13 +5039,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   		}
>>   
>>   		if (!intel_dp->is_mst) {
>> +			bool full_detect;
>>   			/*
>>   			 * we'll check the link status via the normal hot plug path later -
>>   			 * but for short hpds we should check it now
>>   			 */
>>   			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> +			intel_dp_check_link_status(intel_dp, &full_detect);
>>   			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +			if (full_detect)
>> +				goto put_power;
>>   		}
>>   	}
>>   
>> -- 
>> 1.7.9.5
>>

-- 
regards,
Sivakumar

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

Re: [PATCH 3/3] drm/i915: force full detect on sink count change

[Sivakumar Thulasimani]

On 8/26/2015 3:32 PM, Jani Nikula wrote:
> On Tue, 25 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> This patch checks for changes in sink count between short pulse
>> hpds and forces full detect when there is a change. This will
>> result in compliance test case 4.2.2.8 passing since it tests
>> for this behavior.
>>
>> This will allow both detection of hotplug and unplug of panels
>> through dongles that give only short pulse for such events.
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 834f513..eb3ab41 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4370,26 +4370,34 @@ go_again:
>>    *  4. Check link status on receipt of hot-plug interrupt
>>    */
>>   static void
>> -intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
> You could turn perform_full_detect into a boolean return value which
> this function doesn't have.
ok thinking a few mins brings back the reason for this change :).

we return from this function for the following reasons,
 > dpcd read failed
 > sink count change
 > crtc is not active
 > ATR was requested and it was handled/ignored
 > link status was good or if bad we retrained it.
     and it might have passed or failed.

lets say i return true or false. what does that mean ?

i'll need full detection only if display was already present and is not 
anymore
which can be due to
      dpcd read failure while the display was enabled ? (basically sink 
count change from 1 to 0)
or sink count changed from 0 to 1.

so it is better to have an explicit parameter rather than

>
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>   	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	struct intel_crtc *crtc =
>>   		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>>   	u8 sink_irq_vector;
>> +	u8 old_sink_count = intel_dp->sink_count;
>> +	u8 ret;
> Isn't this a bool, really?
>
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> +	*perform_full_detect = false;
>> +
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
>>   	}
>>   
>>   	/* Now read the DPCD to see if it's actually running */
>> -	if (!intel_dp_get_dpcd(intel_dp)) {
>> +	ret = intel_dp_get_dpcd(intel_dp);
>> +
>> +	if (old_sink_count != intel_dp->sink_count)
>> +		*perform_full_detect = true;
> Can't trust intel_dp->sink_count without checking the return value. So I
> guess you can call intel_dp_get_dpcd the way it's done now, and move the
> sink count check below it.
>
> BR,
> Jani.
>
>> +
>> +	if (!ret)
>>   		return;
>> -	}
>>   
>>   	if (!intel_encoder->base.crtc)
>>   		return;
>> @@ -5031,13 +5039,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   		}
>>   
>>   		if (!intel_dp->is_mst) {
>> +			bool full_detect;
>>   			/*
>>   			 * we'll check the link status via the normal hot plug path later -
>>   			 * but for short hpds we should check it now
>>   			 */
>>   			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> +			intel_dp_check_link_status(intel_dp, &full_detect);
>>   			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +			if (full_detect)
>> +				goto put_power;
>>   		}
>>   	}
>>   
>> -- 
>> 1.7.9.5
>>

-- 
regards,
Sivakumar

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

Re: [PATCH 1/3] drm/i915: read sink_count dpcd always

[Ville Syrjälä]

On Tue, Aug 25, 2015 at 05:20:36PM +0530, Sivakumar Thulasimani wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> 
> This patch reads sink_count dpcd always and removes its
> read operation based on values in downstream port dpcd. Also
> we should read it irrespective of current status of sink.
> 
> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
> SINK_COUNT denotes if a display is attached, while
> DOWNSTREAM_PORT_PRESET indicates how many ports are available
> in the dongle where display can be attached. so it is possible
> for sink count to change irrespective of value in downstream
> port dpcd.
> 
> Here is a table of possible values and scenarios
> 
> sink_count	downstream_port
> 		present
> 0		0		no display is attached
> 0		1		dongle is connected without display
> 1		0		display connected directly
> 1		1		display connected through dongle
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8a66a44..9e4e27d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
> +	uint8_t reg;
>  
>  	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>  				    sizeof(intel_dp->dpcd)) < 0)
> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>  		return false; /* DPCD not present */
>  
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +					    &reg, 1) < 0)
> +			return false;
> +
> +	if (!DP_GET_SINK_COUNT(reg))
> +		return false;
> +
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> @@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> -
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> -

I don't like trying to overload intel_dp_check_link_status() to do other
stuff besides link retraining. The sink irq stuff already snuck in
somehow, but I think we should pull it out and try to make the function
names match what they do.

The entire DP hpd locking also needs more though. We now grab the
connection_mutex for the retraining to prevent racing against modeset,
but it doesn't prevent racing against ->detect(). And for MST we take
no locks whatsoever, even for the retraining (I have a patch for that
that I'll send out soon).

>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;
> @@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;
> +
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>  		return connector_status_connected;
>  
> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> -		uint8_t reg;
> -
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &reg, 1) < 0)
> -			return connector_status_unknown;
> -
> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> -					      : connector_status_disconnected;
> -	}
> -
>  	/* If no HPD, poke DDC gently */
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
> -- 
> 1.7.9.5

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

Re: [PATCH 1/3] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

On 8/26/2015 5:21 PM, Ville Syrjälä wrote:
> On Tue, Aug 25, 2015 at 05:20:36PM +0530, Sivakumar Thulasimani wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> This patch reads sink_count dpcd always and removes its
>> read operation based on values in downstream port dpcd. Also
>> we should read it irrespective of current status of sink.
>>
>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>> SINK_COUNT denotes if a display is attached, while
>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>> in the dongle where display can be attached. so it is possible
>> for sink count to change irrespective of value in downstream
>> port dpcd.
>>
>> Here is a table of possible values and scenarios
>>
>> sink_count	downstream_port
>> 		present
>> 0		0		no display is attached
>> 0		1		dongle is connected without display
>> 1		0		display connected directly
>> 1		1		display connected through dongle
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
>>   1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8a66a44..9e4e27d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	struct drm_device *dev = dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	uint8_t rev;
>> +	uint8_t reg;
>>   
>>   	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>   				    sizeof(intel_dp->dpcd)) < 0)
>> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>   		return false; /* DPCD not present */
>>   
>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +					    &reg, 1) < 0)
>> +			return false;
>> +
>> +	if (!DP_GET_SINK_COUNT(reg))
>> +		return false;
>> +
>>   	/* Check if the panel supports PSR */
>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>   	if (is_edp(intel_dp)) {
>> @@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> -
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> -
> I don't like trying to overload intel_dp_check_link_status() to do other
> stuff besides link retraining. The sink irq stuff already snuck in
> somehow, but I think we should pull it out and try to make the function
> names match what they do.
>
> The entire DP hpd locking also needs more though. We now grab the
> connection_mutex for the retraining to prevent racing against modeset,
> but it doesn't prevent racing against ->detect(). And for MST we take
> no locks whatsoever, even for the retraining (I have a patch for that
> that I'll send out soon).
Agree, i am still thinking on how to extract the Automated test request 
part so
we can avoid having same code twice in intel_dp_detect() and in
intel_dp_check_link_status. if your patch also cleans up this function i can
rebase on top of it.
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
>> @@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   		return;
>>   	}
>>   
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
>> +
>>   	/* Try to read the source of the interrupt */
>>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>   	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>   		return connector_status_connected;
>>   
>> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>> -		uint8_t reg;
>> -
>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> -					    &reg, 1) < 0)
>> -			return connector_status_unknown;
>> -
>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> -					      : connector_status_disconnected;
>> -	}
>> -
>>   	/* If no HPD, poke DDC gently */
>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>   		return connector_status_connected;
>> -- 
>> 1.7.9.5

-- 
regards,
Sivakumar

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