On Fri, Jun 02, 2017 at 11:27:40AM +0300, Jani Nikula wrote:
> On Fri, 02 Jun 2017, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
