[PATCH 1/4] drm/i915: Add support for DP link training compliance
Manasi Navare
manasi.d.navare at intel.com
Mon Jan 23 20:50:07 UTC 2017
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.navare at 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.navare at intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Ville Syrjala <ville.syrjala at 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.
> 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
More information about the dri-devel
mailing list