Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@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@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@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().

> +		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.

> +	}
> +	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.

> +
> +	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.

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.

> +
> +	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.

>  }
>  
> @@ -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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel