diff mbox

drm/i915: Splitting intel_dp_check_link_status

Message ID 1453114339-29826-1-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shubhangi Shrivastava Jan. 18, 2016, 10:52 a.m. UTC
When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.
The link retraining part when EQ is not correct is
retained to intel_dp_check_link_status whereas other
operations are handled as part of intel_dp_short_pulse.
This change is required to avoid performing all DPCD
related operations on performing link retraining.

v2: Added WARN_ON to intel_dp_check_link_status()
    Removed a call to intel_dp_get_link_status() (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

Comments

Lukas Wunner Jan. 18, 2016, 9:05 p.m. UTC | #1
Hi,

On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> When created originally intel_dp_check_link_status()
> was supposed to handle only link training for short
> pulse but has grown into handler for short pulse itself.
> This patch cleans up this function by splitting it into
> two halves. First intel_dp_short_pulse() is called,
> which will be entry point and handle all logic for
> short pulse handling while intel_dp_check_link_status()
> will retain its original purpose of only doing link
> status related work.
> The link retraining part when EQ is not correct is
> retained to intel_dp_check_link_status whereas other
> operations are handled as part of intel_dp_short_pulse.
> This change is required to avoid performing all DPCD
> related operations on performing link retraining.
> 
> v2: Added WARN_ON to intel_dp_check_link_status()
>     Removed a call to intel_dp_get_link_status() (Ander)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 82ee18d..f8d9611 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4279,6 +4279,36 @@ go_again:
>  	return -EINVAL;
>  }
>  
> +static void
> +intel_dp_check_link_status(struct intel_dp *intel_dp)
> +{
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	u8 link_status[DP_LINK_STATUS_SIZE];
> +
> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +
> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +		DRM_ERROR("Failed to get link status\n");
> +		return;
> +	}
> +
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;

Why do you change the order of the three if-clauses above?
The original order seems to make more sense. (Checking for
->base.crtc and ->active is cheap, whereas accessing AUX to
get the link status is time consuming. You don't want to
spend that time only to bail out, should one of the other two
if-clauses fail.)

Best regards,

Lukas

> +
> +	/* if link training is requested we should perform it always */
> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> +				intel_encoder->base.name);
> +		intel_dp_start_link_train(intel_dp);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
> +}
> +
>  /*
>   * According to DP spec
>   * 5.1.2:
> @@ -4288,14 +4318,10 @@ 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_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector;
> -	u8 link_status[DP_LINK_STATUS_SIZE];
> -
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
>  	/*
>  	 * Clearing compliance test variables to allow capturing
> @@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	intel_dp->compliance_test_type = 0;
>  	intel_dp->compliance_test_data = 0;
>  
> -	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;
> -	}
> -
>  	/* Now read the DPCD to see if it's actually running */
>  	if (!intel_dp_get_dpcd(intel_dp)) {
>  		return;
> @@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> -		intel_dp_start_link_train(intel_dp);
> -		intel_dp_stop_link_train(intel_dp);
> -	}
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	intel_dp_check_link_status(intel_dp);
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
>  /* XXX this is probably wrong for multiple downstream ports */
> @@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			}
>  		}
>  
> -		if (!intel_dp->is_mst) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		}
> +		if (!intel_dp->is_mst)
> +			intel_dp_short_pulse(intel_dp);
>  	}
>  
>  	ret = IRQ_HANDLED;
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sivakumar Thulasimani Jan. 19, 2016, 4:44 a.m. UTC | #2
On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> Hi,
>
> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>> When created originally intel_dp_check_link_status()
>> was supposed to handle only link training for short
>> pulse but has grown into handler for short pulse itself.
>> This patch cleans up this function by splitting it into
>> two halves. First intel_dp_short_pulse() is called,
>> which will be entry point and handle all logic for
>> short pulse handling while intel_dp_check_link_status()
>> will retain its original purpose of only doing link
>> status related work.
>> The link retraining part when EQ is not correct is
>> retained to intel_dp_check_link_status whereas other
>> operations are handled as part of intel_dp_short_pulse.
>> This change is required to avoid performing all DPCD
>> related operations on performing link retraining.
>>
>> v2: Added WARN_ON to intel_dp_check_link_status()
>>      Removed a call to intel_dp_get_link_status() (Ander)
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 82ee18d..f8d9611 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4279,6 +4279,36 @@ go_again:
>>   	return -EINVAL;
>>   }
>>   
>> +static void
>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>> +
>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> +
>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +		DRM_ERROR("Failed to get link status\n");
>> +		return;
>> +	}
>> +
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
> Why do you change the order of the three if-clauses above?
> The original order seems to make more sense. (Checking for
> ->base.crtc and ->active is cheap, whereas accessing AUX to
> get the link status is time consuming. You don't want to
> spend that time only to bail out, should one of the other two
> if-clauses fail.)
>
> Best regards,
>
> Lukas
Actually it is expected to read link status whenever we receive short 
pulse interrupt
irrespective of the panel being enabled or not. So this change is with 
respect to
that rather than any performance based.
regards,
Sivakumar
>> +
>> +	/* if link training is requested we should perform it always */
>> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> +				intel_encoder->base.name);
>> +		intel_dp_start_link_train(intel_dp);
>> +		intel_dp_stop_link_train(intel_dp);
>> +	}
>> +}
>> +
>>   /*
>>    * According to DP spec
>>    * 5.1.2:
>> @@ -4288,14 +4318,10 @@ 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_short_pulse(struct intel_dp *intel_dp)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	u8 sink_irq_vector;
>> -	u8 link_status[DP_LINK_STATUS_SIZE];
>> -
>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>>   	/*
>>   	 * Clearing compliance test variables to allow capturing
>> @@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   	intel_dp->compliance_test_type = 0;
>>   	intel_dp->compliance_test_data = 0;
>>   
>> -	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;
>> -	}
>> -
>>   	/* Now read the DPCD to see if it's actually running */
>>   	if (!intel_dp_get_dpcd(intel_dp)) {
>>   		return;
>> @@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>>   	}
>>   
>> -	/* if link training is requested we should perform it always */
>> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> -			      intel_encoder->base.name);
>> -		intel_dp_start_link_train(intel_dp);
>> -		intel_dp_stop_link_train(intel_dp);
>> -	}
>> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +	intel_dp_check_link_status(intel_dp);
>> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>   }
>>   
>>   /* XXX this is probably wrong for multiple downstream ports */
>> @@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   			}
>>   		}
>>   
>> -		if (!intel_dp->is_mst) {
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -		}
>> +		if (!intel_dp->is_mst)
>> +			intel_dp_short_pulse(intel_dp);
>>   	}
>>   
>>   	ret = IRQ_HANDLED;
>> -- 
>> 2.6.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Jan. 19, 2016, 8:44 a.m. UTC | #3
On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> >Hi,
> >
> >On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> >>When created originally intel_dp_check_link_status()
> >>was supposed to handle only link training for short
> >>pulse but has grown into handler for short pulse itself.
> >>This patch cleans up this function by splitting it into
> >>two halves. First intel_dp_short_pulse() is called,
> >>which will be entry point and handle all logic for
> >>short pulse handling while intel_dp_check_link_status()
> >>will retain its original purpose of only doing link
> >>status related work.
> >>The link retraining part when EQ is not correct is
> >>retained to intel_dp_check_link_status whereas other
> >>operations are handled as part of intel_dp_short_pulse.
> >>This change is required to avoid performing all DPCD
> >>related operations on performing link retraining.
> >>
> >>v2: Added WARN_ON to intel_dp_check_link_status()
> >>     Removed a call to intel_dp_get_link_status() (Ander)
> >>
> >>Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> >>Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> >>Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
> >>  1 file changed, 36 insertions(+), 29 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>index 82ee18d..f8d9611 100644
> >>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>@@ -4279,6 +4279,36 @@ go_again:
> >>  	return -EINVAL;
> >>  }
> >>+static void
> >>+intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>+{
> >>+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >>+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>+	u8 link_status[DP_LINK_STATUS_SIZE];
> >>+
> >>+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >>+
> >>+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >>+		DRM_ERROR("Failed to get link status\n");
> >>+		return;
> >>+	}
> >>+
> >>+	if (!intel_encoder->base.crtc)
> >>+		return;
> >>+
> >>+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> >>+		return;
> >Why do you change the order of the three if-clauses above?
> >The original order seems to make more sense. (Checking for
> >->base.crtc and ->active is cheap, whereas accessing AUX to
> >get the link status is time consuming. You don't want to
> >spend that time only to bail out, should one of the other two
> >if-clauses fail.)
> >
> >Best regards,
> >
> >Lukas
> Actually it is expected to read link status whenever we receive short pulse
> interrupt
> irrespective of the panel being enabled or not. So this change is with
> respect to
> that rather than any performance based.

As a general rule please don't make functional changes like these in a
patch that just splits stuff up. Your patch summary sounds like simple
refactoring, which this doesn't seem to be.
-Daniel
Sivakumar Thulasimani Jan. 19, 2016, 8:59 a.m. UTC | #4
On 1/19/2016 2:14 PM, Daniel Vetter wrote:
> On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
>>> Hi,
>>>
>>> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>>>> When created originally intel_dp_check_link_status()
>>>> was supposed to handle only link training for short
>>>> pulse but has grown into handler for short pulse itself.
>>>> This patch cleans up this function by splitting it into
>>>> two halves. First intel_dp_short_pulse() is called,
>>>> which will be entry point and handle all logic for
>>>> short pulse handling while intel_dp_check_link_status()
>>>> will retain its original purpose of only doing link
>>>> status related work.
>>>> The link retraining part when EQ is not correct is
>>>> retained to intel_dp_check_link_status whereas other
>>>> operations are handled as part of intel_dp_short_pulse.
>>>> This change is required to avoid performing all DPCD
>>>> related operations on performing link retraining.
>>>>
>>>> v2: Added WARN_ON to intel_dp_check_link_status()
>>>>      Removed a call to intel_dp_get_link_status() (Ander)
>>>>
>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
>>>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 82ee18d..f8d9611 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4279,6 +4279,36 @@ go_again:
>>>>   	return -EINVAL;
>>>>   }
>>>> +static void
>>>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>> +{
>>>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>>>> +
>>>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>> +
>>>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>> +		DRM_ERROR("Failed to get link status\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!intel_encoder->base.crtc)
>>>> +		return;
>>>> +
>>>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>> +		return;
>>> Why do you change the order of the three if-clauses above?
>>> The original order seems to make more sense. (Checking for
>>> ->base.crtc and ->active is cheap, whereas accessing AUX to
>>> get the link status is time consuming. You don't want to
>>> spend that time only to bail out, should one of the other two
>>> if-clauses fail.)
>>>
>>> Best regards,
>>>
>>> Lukas
>> Actually it is expected to read link status whenever we receive short pulse
>> interrupt
>> irrespective of the panel being enabled or not. So this change is with
>> respect to
>> that rather than any performance based.
> As a general rule please don't make functional changes like these in a
> patch that just splits stuff up. Your patch summary sounds like simple
> refactoring, which this doesn't seem to be.
> -Daniel
Understood, will make the appropriate changes and move that to separate 
patch.

regards,
Sivakumar
Daniel Vetter Jan. 19, 2016, 9:05 a.m. UTC | #5
On Tue, Jan 19, 2016 at 02:29:22PM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 1/19/2016 2:14 PM, Daniel Vetter wrote:
> >On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> >>>Hi,
> >>>
> >>>On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> >>>>When created originally intel_dp_check_link_status()
> >>>>was supposed to handle only link training for short
> >>>>pulse but has grown into handler for short pulse itself.
> >>>>This patch cleans up this function by splitting it into
> >>>>two halves. First intel_dp_short_pulse() is called,
> >>>>which will be entry point and handle all logic for
> >>>>short pulse handling while intel_dp_check_link_status()
> >>>>will retain its original purpose of only doing link
> >>>>status related work.
> >>>>The link retraining part when EQ is not correct is
> >>>>retained to intel_dp_check_link_status whereas other
> >>>>operations are handled as part of intel_dp_short_pulse.
> >>>>This change is required to avoid performing all DPCD
> >>>>related operations on performing link retraining.
> >>>>
> >>>>v2: Added WARN_ON to intel_dp_check_link_status()
> >>>>     Removed a call to intel_dp_get_link_status() (Ander)
> >>>>
> >>>>Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> >>>>Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> >>>>Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
> >>>>  1 file changed, 36 insertions(+), 29 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>>index 82ee18d..f8d9611 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>@@ -4279,6 +4279,36 @@ go_again:
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>+static void
> >>>>+intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>>>+{
> >>>>+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >>>>+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>+	u8 link_status[DP_LINK_STATUS_SIZE];
> >>>>+
> >>>>+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >>>>+
> >>>>+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >>>>+		DRM_ERROR("Failed to get link status\n");
> >>>>+		return;
> >>>>+	}
> >>>>+
> >>>>+	if (!intel_encoder->base.crtc)
> >>>>+		return;
> >>>>+
> >>>>+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> >>>>+		return;
> >>>Why do you change the order of the three if-clauses above?
> >>>The original order seems to make more sense. (Checking for
> >>>->base.crtc and ->active is cheap, whereas accessing AUX to
> >>>get the link status is time consuming. You don't want to
> >>>spend that time only to bail out, should one of the other two
> >>>if-clauses fail.)
> >>>
> >>>Best regards,
> >>>
> >>>Lukas
> >>Actually it is expected to read link status whenever we receive short pulse
> >>interrupt
> >>irrespective of the panel being enabled or not. So this change is with
> >>respect to
> >>that rather than any performance based.
> >As a general rule please don't make functional changes like these in a
> >patch that just splits stuff up. Your patch summary sounds like simple
> >refactoring, which this doesn't seem to be.
> >-Daniel
> Understood, will make the appropriate changes and move that to separate
> patch.

btw you don't have to split it since really this is a small change.
Changing the subject to something that makes is clearer that it's not just
refactoring is also ok, e.g. "reorganize intel_dp_detect"

Then explain in the commit message why and what changes, like you do
already.
-Daniel
Sivakumar Thulasimani Jan. 19, 2016, 9:11 a.m. UTC | #6
On 1/19/2016 2:35 PM, Daniel Vetter wrote:
> On Tue, Jan 19, 2016 at 02:29:22PM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 1/19/2016 2:14 PM, Daniel Vetter wrote:
>>> On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
>>>> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>>>>>> When created originally intel_dp_check_link_status()
>>>>>> was supposed to handle only link training for short
>>>>>> pulse but has grown into handler for short pulse itself.
>>>>>> This patch cleans up this function by splitting it into
>>>>>> two halves. First intel_dp_short_pulse() is called,
>>>>>> which will be entry point and handle all logic for
>>>>>> short pulse handling while intel_dp_check_link_status()
>>>>>> will retain its original purpose of only doing link
>>>>>> status related work.
>>>>>> The link retraining part when EQ is not correct is
>>>>>> retained to intel_dp_check_link_status whereas other
>>>>>> operations are handled as part of intel_dp_short_pulse.
>>>>>> This change is required to avoid performing all DPCD
>>>>>> related operations on performing link retraining.
>>>>>>
>>>>>> v2: Added WARN_ON to intel_dp_check_link_status()
>>>>>>      Removed a call to intel_dp_get_link_status() (Ander)
>>>>>>
>>>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
>>>>>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 82ee18d..f8d9611 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -4279,6 +4279,36 @@ go_again:
>>>>>>   	return -EINVAL;
>>>>>>   }
>>>>>> +static void
>>>>>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>>> +{
>>>>>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>>>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>>>>>> +
>>>>>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>>>> +
>>>>>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>>>> +		DRM_ERROR("Failed to get link status\n");
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!intel_encoder->base.crtc)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>>>> +		return;
>>>>> Why do you change the order of the three if-clauses above?
>>>>> The original order seems to make more sense. (Checking for
>>>>> ->base.crtc and ->active is cheap, whereas accessing AUX to
>>>>> get the link status is time consuming. You don't want to
>>>>> spend that time only to bail out, should one of the other two
>>>>> if-clauses fail.)
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Lukas
>>>> Actually it is expected to read link status whenever we receive short pulse
>>>> interrupt
>>>> irrespective of the panel being enabled or not. So this change is with
>>>> respect to
>>>> that rather than any performance based.
>>> As a general rule please don't make functional changes like these in a
>>> patch that just splits stuff up. Your patch summary sounds like simple
>>> refactoring, which this doesn't seem to be.
>>> -Daniel
>> Understood, will make the appropriate changes and move that to separate
>> patch.
> btw you don't have to split it since really this is a small change.
> Changing the subject to something that makes is clearer that it's not just
> refactoring is also ok, e.g. "reorganize intel_dp_detect"
>
> Then explain in the commit message why and what changes, like you do
> already.
> -Daniel
Sure, that will save some time in redoing ULT+upstreaming :).
to give some background, the movement was supposed to be a separate
patch but got merged during this cleanup. Will make sure that gets
documented and split clearly as required hence forth.

regards,
Sivakumar
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 82ee18d..f8d9611 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4279,6 +4279,36 @@  go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* if link training is requested we should perform it always */
+	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
+		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4288,14 +4318,10 @@  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_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
-	u8 link_status[DP_LINK_STATUS_SIZE];
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4305,17 +4331,6 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	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;
-	}
-
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -4335,14 +4350,9 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
-		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5072,11 +5082,8 @@  intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
+		if (!intel_dp->is_mst)
+			intel_dp_short_pulse(intel_dp);
 	}
 
 	ret = IRQ_HANDLED;