[Intel-gfx] [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Jani Nikula]

On Fri, 02 Jun 2017, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare at intel.com> wrote:
>> Validate the compliance test link parameters when the compliance
>> test dpcd registers are read. Also validate them in compute_config
>> before using them since the max values might have been reduced
>> due to link training fallback.
>>
>> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++----------------
>>  1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 832786d..cda0f0a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1678,12 +1678,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>>  		int index;
>>  
>> -		index = intel_dp_rate_index(intel_dp->common_rates,
>> -					    intel_dp->num_common_rates,
>> -					    intel_dp->compliance.test_link_rate);
>> -		if (index >= 0)
>> +		/* Validate the compliance test data */

Oh, and please do cite the reason for doing this again in the comment. I
first read the code, didn't get it, until I read the commit message. :)

BR,
Jani.


>> +		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
>> +					       intel_dp->compliance.test_lane_count)) {
>> +			index = intel_dp_rate_index(intel_dp->common_rates,
>> +						    intel_dp->num_common_rates,
>> +						    intel_dp->compliance.test_link_rate);
>
> I think you still need to check index >= 0. intel_dp_link_params_valid
> only checks the boundaries, not that the common rates actually contains
> the value.
>
>>  			min_clock = max_clock = index;
>> -		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
>> +			min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
>> +		}
>>  	}
>>  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
>>  		      "max bw %d pixel clock %iKHz\n",
>> @@ -3961,8 +3964,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>>  {
>>  	int status = 0;
>> -	int min_lane_count = 1;
>> -	int link_rate_index, test_link_rate;
>> +	int test_link_rate;
>>  	uint8_t test_lane_count, test_link_bw;
>>  	/* (DP CTS 1.2)
>>  	 * 4.3.1.11
>> @@ -3976,10 +3978,6 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>>  		return DP_TEST_NAK;
>>  	}
>>  	test_lane_count &= DP_MAX_LANE_COUNT_MASK;
>> -	/* Validate the requested lane count */
>> -	if (test_lane_count < min_lane_count ||
>> -	    test_lane_count > intel_dp->max_link_lane_count)
>> -		return DP_TEST_NAK;
>>  
>>  	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
>>  				   &test_link_bw);
>> @@ -3987,12 +3985,11 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>>  		DRM_DEBUG_KMS("Link Rate read failed\n");
>>  		return DP_TEST_NAK;
>>  	}
>> -	/* Validate the requested link rate */
>>  	test_link_rate = drm_dp_bw_code_to_link_rate(test_link_bw);
>> -	link_rate_index = intel_dp_rate_index(intel_dp->common_rates,
>> -					      intel_dp->num_common_rates,
>> -					      test_link_rate);
>> -	if (link_rate_index < 0)
>> +
>> +	/* Validate the requested link rate and lane count */
>> +	if (!intel_dp_link_params_valid(intel_dp, test_link_rate,
>> +					test_lane_count))
>>  		return DP_TEST_NAK;
>
> The changes here LGTM.
>
>>  
>>  	intel_dp->compliance.test_lane_count = test_lane_count;

-- 
Jani Nikula, Intel Open Source Technology Center