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

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

On Thu, 24 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Wed, Nov 23, 2016 at 03:07:30PM +0200, Jani Nikula wrote:
>> On Tue, 22 Nov 2016, 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.
>> >
>> > 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  | 73 ++++++++++++++++++++++++++++++++++++----
>> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>> >  2 files changed, 69 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 90283ed..69944d1 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -288,6 +288,21 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
>> >  			       common_rates);
>> >  }
>> >  
>> > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
>> > +				    int *common_rates, int link_rate)
>> > +{
>> > +	int common_len;
>> > +	int index;
>> > +
>> > +	common_len = intel_dp_common_rates(intel_dp, common_rates);
>> > +	for (index = 0; index < common_len; index++) {
>> > +		if (link_rate == common_rates[common_len - index - 1])
>> > +			return common_len - index - 1;
>> > +	}
>> > +
>> > +	return -1;
>> > +}
>> > +
>> >  static enum drm_mode_status
>> >  intel_dp_mode_valid(struct drm_connector *connector,
>> >  		    struct drm_display_mode *mode)
>> > @@ -1554,6 +1569,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] = {};
>> > @@ -1595,6 +1611,16 @@ 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));
>> > +			if (link_rate_index >= 0)
>> > +				min_clock = max_clock = link_rate_index;
>> > +			min_lane_count = max_lane_count = intel_dp->compliance_test_lane_count;
>> 
>> You need to be more strict about validating
>> compliance_test_lane_count. You do mask it with DP_MAX_LANE_COUNT_MASK,
>> but that's 0x1f, quite a few more lanes than we have...
>>
>
> So the reason I didnt add validation here is because we enter the DUT
> capabilities into DPR-120 before starting the test, so the test lane
> count its gonna request will not exceed the DUT max lane count.  But
> we can still take safe approach and take the min between the
> compliance_lane_count and max source lane count.

The kernel must validate any and all input whether it comes from
userspace or from other devices.

>> > +	}
>> > +
>> >  	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],
>> > @@ -1642,6 +1668,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>> >  				}
>> >  			}
>> >  		}
>> > +
>> 
>> Please pay attention to not making unrelated changes.
>> 
>> >  	}
>> >  
>> >  	return false;
>> > @@ -3804,6 +3831,29 @@ 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;
>> > +	int status = 0;
>> > +	/* (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("Could not read test lane count from "
>> > +			      "reference sink\n");
>> 
>> No need to be so verbose, DRM_DEBUG_KMS will include the function name,
>> so a simple "Lane count read failed" or something will suffice.
>> 
>> > +		return 0;
>> 
>> Should these return DP_TEST_NAK on errors or what?
>>
>
> No in this case it should just return which will not send any test
> response and the test will timeout after 5 retries and stop.
> TEST_NAK is usually if the test is not supported.

Ok.

>
>
>> > +	}
>> > +	intel_dp->compliance_test_lane_count &= DP_MAX_LANE_COUNT_MASK;
>> > +
>> > +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
>> > +				   &intel_dp->compliance_test_link_rate);
>> > +	if (status <= 0) {
>> > +		DRM_DEBUG_KMS("Could not read test link rate from "
>> > +			      "refernce sink\n");
>> 
>> Ditto.
>> 
>> > +		return 0;
>> > +	}
>> > +
>> >  	return test_result;
>> >  }
>> >  
>> > @@ -3908,7 +3958,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> >  				   DP_TEST_RESPONSE,
>> >  				   &response, 1);
>> >  	if (status <= 0)
>> > -		DRM_DEBUG_KMS("Could not write test response to sink\n");
>> > +		DRM_DEBUG_KMS("Could not write test response "
>> > +			      "to sink\n");
>> 
>> Unrelated change, and one we don't want.
>> 
>> >  }
>> >  
>> >  static int
>> > @@ -4018,9 +4069,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> >  	if (WARN_ON_ONCE(!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))) {
>> 
>> Too many braces.
>> 
>> >  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> >  			      intel_encoder->base.name);
>> >  
>> > @@ -4045,6 +4095,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;
>> > @@ -4056,6 +4107,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> >  	intel_dp->compliance_test_active = 0;
>> >  	intel_dp->compliance_test_type = 0;
>> >  	intel_dp->compliance_test_data = 0;
>> > +	intel_dp->compliance_test_lane_count = 0;
>> > +	intel_dp->compliance_test_link_rate = 0;
>> 
>> Looks like compliance stuff should be a sub struct in intel_dp, and you
>> could just memset it to 0.
>>
>
> Yes I can add a struct for this.
>  
>> >  
>> >  	/*
>> >  	 * Now read the DPCD to see if it's actually running
>> > @@ -4079,8 +4132,9 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> >  				   DP_DEVICE_SERVICE_IRQ_VECTOR,
>> >  				   sink_irq_vector);
>> >  
>> > -		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>> > -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
>> > +		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) {
>> > +			intel_dp_handle_test_request(intel_dp);
>> > +		}
>> 
>> Unnecessary curly braces.
>> 
>> >  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>> >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>> >  	}
>> > @@ -4088,6 +4142,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)) {
>> 
>> Too many braces.
>> 
>> > +		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;
>> >  }
>> > @@ -4375,6 +4434,8 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>> >  		intel_dp->compliance_test_active = 0;
>> >  		intel_dp->compliance_test_type = 0;
>> >  		intel_dp->compliance_test_data = 0;
>> > +		intel_dp->compliance_test_lane_count = 0;
>> > +		intel_dp->compliance_test_link_rate = 0;
>> 
>> Same thing about making compliance sub struct.
>> 
>> >  
>> >  		if (intel_dp->is_mst) {
>> >  			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index cd132c2..1e88288 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -958,6 +958,8 @@ struct intel_dp {
>> >  	unsigned long compliance_test_type;
>> >  	unsigned long compliance_test_data;
>> >  	bool compliance_test_active;
>> > +	u8 compliance_test_lane_count;
>> > +	u8 compliance_test_link_rate;
>> >  };
>> >  
>> >  struct intel_lspcon {
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel