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

Message ID 1440503438-13470-4-git-send-email-sivakumar.thulasimani@intel.com
State New
Headers show

Commit Message

Sivakumar Thulasimani Aug. 25, 2015, 11:50 a.m. UTC
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(-)

Comments

Jani Nikula Aug. 26, 2015, 10:02 a.m. UTC | #1
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
>
Sivakumar Thulasimani Aug. 26, 2015, 10:39 a.m. UTC | #2
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
>>
Sivakumar Thulasimani Aug. 26, 2015, 11:23 a.m. UTC | #3
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
>>

Patch
diff mbox

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