| Message ID | 1496364687-30660-2-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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;
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
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
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
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
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;
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(-)