[PATCH 1/2] drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated

[PATCH 1/2] drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated

[Manasi Navare]

This function now takes the link rate and lane ocunt to be validated
as an argument so that this can be used for validating even the
compliance test link parameters.

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 58dca87..832786d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -322,19 +322,20 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
-static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
+				       uint8_t lane_count)
 {
 	/*
 	 * FIXME: we need to synchronize the current link parameters with
 	 * hardware readout. Currently fast link training doesn't work on
 	 * boot-up.
 	 */
-	if (intel_dp->link_rate == 0 ||
-	    intel_dp->link_rate > intel_dp->max_link_rate)
+	if (link_rate == 0 ||
+	    link_rate > intel_dp->max_link_rate)
 		return false;
 
-	if (intel_dp->lane_count == 0 ||
-	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+	if (lane_count == 0 ||
+	    lane_count > intel_dp_max_lane_count(intel_dp))
 		return false;
 
 	return true;
@@ -4260,7 +4261,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	 * Validate the cached values of intel_dp->link_rate and
 	 * intel_dp->lane_count before attempting to retrain.
 	 */
-	if (!intel_dp_link_params_valid(intel_dp))
+	if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
+					intel_dp->lane_count))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */
-- 
2.1.4

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

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

[Manasi Navare]

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@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 */
+		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);
 			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;
 
 	intel_dp->compliance.test_lane_count = test_lane_count;
-- 
2.1.4

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

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Jani Nikula]

On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@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@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 */
> +		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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Jani Nikula]

On Fri, 02 Jun 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Manasi Navare]

On Fri, Jun 02, 2017 at 11:55:32AM +0300, Jani Nikula wrote:
> On Fri, 02 Jun 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@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@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.
>

Thanks for the review. Will add this reason in the comment.
Will send a new revision with this change and checking index >=0.

Regards
Manasi

 
> 
> >> +		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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Manasi Navare]

On Fri, Jun 02, 2017 at 11:27:40AM +0300, Jani Nikula wrote:
> On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@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@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 */
> > +		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.
>

I thought that it would be implied that if it falls within the bounds then
it would be part of the common_rates array. But yes no harm in checking the index >=0 here
again. 

I will add this check.

Manasi

 
> >  			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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:51:27PM -0700, Manasi Navare 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@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 */
> +		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);
>  			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;

This looks a bit questionable. What if eg. just the link_rate is out of
bounds but the lane count is good?

I think this could be solved in a different way with the patch I posted
maybe one year ago that allowed rate_index() to return a non-exact match,
and by clamping the 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;
>  
>  	intel_dp->compliance.test_lane_count = test_lane_count;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm/i915/dp: Validate the compliance test link parameters

[Manasi Navare]

On Fri, Jun 02, 2017 at 01:34:10PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 01, 2017 at 05:51:27PM -0700, Manasi Navare 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@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 */
> > +		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);
> >  			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;
> 
> This looks a bit questionable. What if eg. just the link_rate is out of
> bounds but the lane count is good?
>

Hi Ville,

Thanks for your feedback/concern here.
But since the test requests both link rate and lane count to be used for the
compliance run, even if one of the parameters are out of bound then we need to bail
from using that combination in the compliance test run and instead go with the
fallback values.

Thought?

Manasi
 
> I think this could be solved in a different way with the patch I posted
> maybe one year ago that allowed rate_index() to return a non-exact match,
> and by clamping the 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;
> >  
> >  	intel_dp->compliance.test_lane_count = test_lane_count;
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated
URL   : https://patchwork.freedesktop.org/series/25191/
State : success

== Summary ==

Series 25191v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/25191/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-snb-2520m) fdo#101048

fdo#101048 https://bugs.freedesktop.org/show_bug.cgi?id=101048

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:447s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:440s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:576s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:508s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:482s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:488s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:472s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:574s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:463s
fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:433s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:468s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:495s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:531s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:401s

2c9abf8ec88cf4b3d3735976bb0ff7a4991946b2 drm-tip: 2017y-06m-01d-13h-29m-05s UTC integration manifest
dc0322a drm/i915/dp: Validate the compliance test link parameters
b121fbf drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4864/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/dp: Generalize intel_dp_link_params function to accept arguments to be validated

[Jani Nikula]

On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> This function now takes the link rate and lane ocunt to be validated
> as an argument so that this can be used for validating even the
> compliance test link parameters.
>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 58dca87..832786d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -322,19 +322,20 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
>  	return 0;
>  }
>  
> -static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
> +static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
> +				       uint8_t lane_count)
>  {
>  	/*
>  	 * FIXME: we need to synchronize the current link parameters with
>  	 * hardware readout. Currently fast link training doesn't work on
>  	 * boot-up.
>  	 */
> -	if (intel_dp->link_rate == 0 ||
> -	    intel_dp->link_rate > intel_dp->max_link_rate)
> +	if (link_rate == 0 ||
> +	    link_rate > intel_dp->max_link_rate)
>  		return false;
>  
> -	if (intel_dp->lane_count == 0 ||
> -	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
> +	if (lane_count == 0 ||
> +	    lane_count > intel_dp_max_lane_count(intel_dp))
>  		return false;
>  
>  	return true;
> @@ -4260,7 +4261,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	 * Validate the cached values of intel_dp->link_rate and
>  	 * intel_dp->lane_count before attempting to retrain.
>  	 */
> -	if (!intel_dp_link_params_valid(intel_dp))
> +	if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
> +					intel_dp->lane_count))
>  		return;
>  
>  	/* Retrain if Channel EQ or CR not ok */

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