On Mon, Jan 23, 2017 at 12:50:07PM -0800, Manasi Navare wrote:
> Thanks Jani for reviewing this patch and for your feedback.
> Its very useful feedback and below are some of my comments:
>
> On Fri, Jan 20, 2017 at 05:05:03PM +0200, Jani Nikula wrote:
> > On Fri, 20 Jan 2017, Manasi Navare <manasi.d.nav...@intel.com> wrote:
> > > This patch adds support to handle automated DP compliance
> > > link training test requests. This patch has been tested with
> > > Unigraf DPR-120 DP Compliance device for testing Link
> > > Training Compliance.
> > > After we get a short pulse Compliance test request, test
> > > request values are read and hotplug uevent is sent in order
> > > to trigger another modeset during which the pipe is configured
> > > and link is retrained and enabled for link parameters requested
> > > by the test.
> > >
> > > v3:
> > > * Validate the test link rate and lane count as soon as
> > > the erquest comes (Jani Nikula)
> > > v2:
> > > * Validate the test lane count before using it in
> > > intel_dp_compute_config (Jani Nikula)
> > > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> > > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vet...@intel.com>
> > > Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 60
> > > ++++++++++++++++++++++++++++++++++++----
> > > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > > 2 files changed, 57 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index e80d620..3e76b63 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp
> > > *intel_dp,
> > > /* Conveniently, the link BW constants become indices with a shift...*/
> > > int min_clock = 0;
> > > int max_clock;
> > > + int link_rate_index;
> > > int bpp, mode_rate;
> > > int link_avail, link_clock;
> > > int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > > @@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp
> > > *intel_dp,
> > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > return false;
> > >
> > > + /* Use values requested by Compliance Test Request */
> > > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > > + link_rate_index = intel_dp_link_rate_index(intel_dp,
> > > + common_rates,
> > > +
> > > drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> >
> > You should probably store the link *rate* instead of the link rate
> > *code* to intel_dp->compliance.test_link_rate in
> > intel_dp_autotest_link_training().
> >
>
> I totally agree and this is how it is addressed in the new revision.
> I have declared a local variable test_link_bw that stores the BW code
> read from the DPCD test registers. This BW code is then validated to see
> that it corresponds to the valid link rate values. If it is invalid then I
> return
> a TEST_NAK and if it is valid then I convert this BW code to link_rate
> and store the link_rate in intel_dp->compliance.test_link_rate.
>
> > > + if (link_rate_index >= 0)
> > > + min_clock = max_clock = link_rate_index;
> > > + 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",
> > > max_lane_count, common_rates[max_clock],
> > > @@ -3920,7 +3930,42 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp,
> > > u8 *crc)
> > >
> > > static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> > > {
> > > - uint8_t test_result = DP_TEST_ACK;
> > > + uint8_t test_result = DP_TEST_NAK;
> > > + int status = 0;
> > > + int min_lane_count = 1;
> > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > > + int link_rate_index;
> > > + /* (DP CTS 1.2)
> > > + * 4.3.1.11
> > > + */
> > > + /* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> > > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> > > + &intel_dp->compliance.test_lane_count);
> > > +
> > > + if (status <= 0) {
> > > + DRM_DEBUG_KMS("Lane count read failed\n");
> > > + return 0;
> >
> > So if you fail to read the lane count, you return 0, write that to
> > TEST_RESPONSE, which is supposed to have no effect on TEST_REQ state per
> > the spec, i.e. writing 0 is useless. intel_dp->compliance.test_type is
> > set anyway, which will try to use the (stale) lane count and link rate
> > values.
> >
>
> I agree that writing a 0 to test_result has no effect to the test response and
> there is a possibility of using stale values of lane count and link rate.
> So now in the new revision, I am returning a TEST_NAK in case of both read
> failures
> as well as the invalid values.
> As per the new patch you have submitted, once intel_dp_handle_test_request()
> recieves
> a TEST_NAK it should not set the test_type and it will eliminate the
> possibility
> of using stale values of link rate and lane count.
>
>
> > > + }
> > > + intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> > > + /* Validate the requested lane count */
> > > + if (intel_dp->compliance.test_lane_count < min_lane_count ||
> > > + intel_dp->compliance.test_lane_count >
> > > intel_dp->max_sink_lane_count)
> > > + return test_result;
> >
> > But if the lane count is out of bounds, you return NAK and write that to
> > TEST_RESPONSE, *but* set intel_dp->compliance.test_type and
> > intel_dp->compliance.test_lane_count anyway, and will try to use the
> > values later on anyway.
> >
>
> This has been addressed in the new revision by storing the lane count read
> from DPCD test registers first into a local test_lane_count variable. This
> values is then validated and made sure it is not out of bounds. If it is
> invalid,
> I return a TEST_NAK so test_type will not be set and this values will not be
> written to
> intel_dp->compliance.test_lane_count. If this value is within bounds, only
> then
> I go ahead and populate the intel_dp->compliance.test_lane_count field.
>
>
> > > +
> > > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> > > + &intel_dp->compliance.test_link_rate);
> > > + if (status <= 0) {
> > > + DRM_DEBUG_KMS("Link Rate read failed\n");
> > > + return 0;
> > > + }
> > > + /* Validate the requested link rate */
> > > + link_rate_index = intel_dp_link_rate_index(intel_dp,
> > > + common_rates,
> > > +
> > > drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> > > + if (link_rate_index < 0)
> > > + return test_result;
> >
> > Same as above for lane count, in both error scenarios.
> >
> > You should probably read both lane count and link rate to local
> > variables first, bail out on read failures, check the values, bail out
> > on invalid values, then set the values in intel_dp->compliance.
> >
>
> Yes this is exactly what the new revision does.
>
Taking one more look at it, I think I should defer populating compliance test
values
until all the validating is done. So I can move setting the
compliance.test_lane_count
and compliance.test_link_rate to the end of the function just before returning
a TEST_ACK.
The reason that this was not addressed earlier was because I assumed that we
were invalidating
the compliance state on NAKs anyway by not setting test_type. But to be on the
safe side
I will move all the compliance test data setting at the end of the function.
This means even for video pattern test handler I should read all the video
pattern data
into local variables first and then set the compliance video pattern test data
values only
at the end of the function just before returning a TEST ACK.
Regards
Manasi
> > intel_dp_handle_test_request() should probably be fixed to not set
> > intel_dp->compliance.test_type on errors. Presumably it should not write
> > zero values to TEST_RESPONSE because it should have no effect anyway;
> > but I'm not sure if we should return 0 from the autotest functions
> > anyway.
> >
>
> Yes, we should not return 0 from the autotest functions. So I am only
> returning
> either a NAK on read failures or invalid values or a TEST_ACK in case of
> success.
>
>
> > > +
> > > + test_result = DP_TEST_ACK;
> > > return test_result;
> >
> > Please tell me what purpose does the test_result variable have in this
> > function. I thought I said you could get rid of the variable.
> >
> > BR,
> > Jani.
> >
>
> The whole purpose of adding a variable named test_result for readability.
> I think that instead of returning NAK and ACK in differnt scenarios, its more
> readable to return test_result instead. This variable can be set to NAK or ACK
> as per the failures or success.
> That is the reason why I have still kept this variable.
>
> Regards
> Manasi
>
>
> > > }
> > >
> > > @@ -4135,9 +4180,8 @@ static void intel_dp_handle_test_request(struct
> > > intel_dp *intel_dp)
> > > if (!intel_dp->lane_count)
> > > 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))) {
> > > + /* Retrain if Channel EQ or CR not ok */
> > > + if (!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);
> > >
> > > @@ -4162,6 +4206,7 @@ static void intel_dp_handle_test_request(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 = 0;
> > > u8 old_sink_count = intel_dp->sink_count;
> > > bool ret;
> > > @@ -4195,7 +4240,7 @@ static void intel_dp_handle_test_request(struct
> > > intel_dp *intel_dp)
> > > sink_irq_vector);
> > >
> > > if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> > > - DRM_DEBUG_DRIVER("Test request in short pulse not
> > > handled\n");
> > > + intel_dp_handle_test_request(intel_dp);
> > > if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
> > > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> > > }
> > > @@ -4203,6 +4248,11 @@ static void intel_dp_handle_test_request(struct
> > > intel_dp *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);
> > > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > > + DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> > > + /* Send a Hotplug Uevent to userspace to start modeset */
> > > + drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> > > + }
> > >
> > > return true;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0cec001..1586a02 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -894,6 +894,8 @@ struct intel_dp_compliance {
> > > unsigned long test_type;
> > > struct intel_dp_compliance_data test_data;
> > > bool test_active;
> > > + u8 test_link_rate;
> > > + u8 test_lane_count;
> > > };
> > >
> > > struct intel_dp {
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
