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

[Manasi Navare <manasi.d.navare@intel.com>]

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.

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


> > +	}
> > +	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx