[Intel-gfx] [PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status

[Shubhangi Shrivastava]

On Wednesday 13 January 2016 08:34 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +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.
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu at intel.com>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++----------------
>> -
>>   1 file changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 137757b..842790e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4289,6 +4289,33 @@ 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;
>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>> +
>> +	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:
>> @@ -4298,15 +4325,12 @@ 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));
>> -
> I think it's better to move this WARN to the new intel_dp_check_link_status().

Sure.. Done..

>>   	/*
>>   	 * Clearing compliance test variables to allow capturing
>>   	 * of values for next automated test request.
>> @@ -4315,12 +4339,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;
> There is now two calls to intel_dp_get_link_status()and the value of link_status
> is not used in this function, so maybe just remove it from here. Looks good
> otherwise.
>
> Ander

Sure.. Done..

>> @@ -4345,14 +4363,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 */
>> @@ -5080,11 +5093,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;