[PATCH 0/5] Add Automation support for DP compliance testing

[PATCH 0/5] Add Automation support for DP compliance testing

[Manasi Navare]

DP 1.2 compliance testing can be acheived using DPR-120's CTS suite.
This compliance unit sends a short pulse to initiate link training
and video pattern generation compliance tests and sends a long pulse
to initate EDID compliance tests. It also sets the AUTOMATED TEST REQUEST
bit in Device IRQ to 1. These patches add support in the kernel to respond
to these test requests sent by DPR-120. The test handler has been added for
Link training/EDID and Video pattern compliance tests that gets invoked on
short/long pulse sent by DPR-120. The test handler for each of the tests
reads the corresponding DPCD registers to read the test parameters.

These tests need to be run with the IGT DP compliance automation tool.
Link Training Tests (4.3.1.1 - 4.3.2.3):
It reads the DPCD registers to get the link rate and lane count requested by
test and sends a hotplug uevent for the userspace to redo a modeset in order
to train the link at the requested test parameters.

EDID Tests (4.2.2.3 - 4.2.2.6):
It reads the EDID set by the DPR-120, if EDID read succeeds, it sets the
video mode to PREFERRED and sets the test_active flag. This flag wakes up
the IGT compliance tool that then fills the framebuffers and triggers a 
modeset.

Video Pattern Tests (4.3.3.1):
It reads the DPCD registers to set the requested video pattern parameters.
It then sets the test active flag that wakes up the IGT tool, userspace
reads the video pattern values from corresponding debugfs files and fills the
framebuffers and triggers a modeset.

Jim Bride (1):
  Add support for forcing 6 bpc on DP pipes.

Manasi Navare (4):
  drm/i915: Add support for DP link training compliance
  drm/i915: Fixes to support DP Compliance EDID tests
  drm: Add definitions for DP compliance Video pattern tests
  drm/i915: Add support for DP Video pattern compliance tests

 drivers/gpu/drm/i915/i915_debugfs.c  |  14 +++-
 drivers/gpu/drm/i915/intel_display.c |   4 +-
 drivers/gpu/drm/i915/intel_dp.c      | 155 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp_mst.c  |  11 ++-
 drivers/gpu/drm/i915/intel_drv.h     |  12 +++
 include/drm/drm_dp_helper.h          |  16 +++-
 6 files changed, 199 insertions(+), 13 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Manasi Navare]

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;
+	}
+
 	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,
 				}
 			}
 		}
+
 	}
 
 	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");
+		return 0;
+	}
+	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");
+		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");
 }
 
 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))) {
 		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;
 
 	/*
 	 * 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);
+		}
 		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)) {
+		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;
 
 		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 {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

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

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

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

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

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

[Manasi Navare]

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

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

[Jani Nikula]

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

[PATCH 2/5] drm/i915: Fixes to support DP Compliance EDID tests

[Manasi Navare]

This patch addresses a few issues from the original patch for
DP Compliance EDID test support submitted by
Todd Previte<todd.previte@gmail.com>

Video Mode requested in the EDID test handler for the EDID Read
test (CTS 4.2.2.3) should be set to PREFERRED as per the CTS spec.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 69944d1..6693918 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3865,7 +3865,7 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_result = DP_TEST_ACK;
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct drm_connector *connector = &intel_connector->base;
 
@@ -3900,7 +3900,7 @@ static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 			DRM_DEBUG_KMS("Failed to write EDID checksum\n");
 
 		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
-		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_PREFERRED;
 	}
 
 	/* Set test active flag here so userspace doesn't interrupt things */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] Add support for forcing 6 bpc on DP pipes.

[Manasi Navare]

From: Jim Bride <jim.bride@linux.intel.com>

For DP compliance we need to be able to control the output color
type for the pipe associated with the DP port. When a specific DP
compliance test is requested with specific BPC value, we read the BPC
value from the DPCD register and store it in the intel_dp structure.
If this BPC value in intel_dp structure  has a non-zero value
and we're on a display port connector, then we use the value to
calculate the bpp for the pipe.  For cases where we are
not on DP or there has not been an overridden value then we behave
as normal.

Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
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_display.c |  4 +++-
 drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 11 +++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7a7ed8..bb1cca2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12694,11 +12694,13 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 
 	/* Clamp display bpp to EDID value */
 	for_each_connector_in_state(state, connector, connector_state, i) {
+
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
 		connected_sink_compute_bpp(to_intel_connector(connector),
-					   pipe_config);
+						   pipe_config);
+
 	}
 
 	return bpp;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6693918..f93e130 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1549,6 +1549,13 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (bpc > 0)
 		bpp = min(bpp, 3*bpc);
 
+	/* For DP Compliance we override the computed bpp for the pipe */
+	if (intel_dp->compliance_force_bpc != 0) {
+		pipe_config->pipe_bpp =	intel_dp->compliance_force_bpc*3;
+		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
+			      pipe_config->pipe_bpp);
+	}
+
 	return bpp;
 }
 
@@ -1629,6 +1636,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	/* Walk through all bpp values. Luckily they're all nicely spaced with 2
 	 * bpc in between. */
 	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
+
 	if (is_edp(intel_dp)) {
 
 		/* Get bpp from vbt only for panels that dont have bpp in edid */
@@ -4109,6 +4117,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_data = 0;
 	intel_dp->compliance_test_lane_count = 0;
 	intel_dp->compliance_test_link_rate = 0;
+	intel_dp->compliance_force_bpc = 0;
 
 	/*
 	 * Now read the DPCD to see if it's actually running
@@ -4434,6 +4443,7 @@ 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_force_bpc = 0;
 		intel_dp->compliance_test_lane_count = 0;
 		intel_dp->compliance_test_link_rate = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index b029d10..940f173 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -45,6 +45,14 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->has_pch_encoder = false;
 	bpp = 24;
+	/* For DP Compliance we need to ensurethat we can override
+	 * the computed bpp for the pipe
+	 */
+	if (intel_dp->compliance_force_bpc) {
+		bpp = intel_dp->compliance_force_bpc * 3;
+		DRM_DEBUG_KMS("Setting pipe bpp to %d\n",
+			      bpp);
+	}
 	/*
 	 * for MST we always configure max link bw - the spec doesn't
 	 * seem to suggest we should do otherwise.
@@ -52,8 +60,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
 
 	pipe_config->lane_count = lane_count;
-
-	pipe_config->pipe_bpp = 24;
+	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
 	state = pipe_config->base.state;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1e88288..3eb428e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -960,6 +960,7 @@ struct intel_dp {
 	bool compliance_test_active;
 	u8 compliance_test_lane_count;
 	u8 compliance_test_link_rate;
+	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
 };
 
 struct intel_lspcon {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] Add support for forcing 6 bpc on DP pipes.

[Jani Nikula]

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> From: Jim Bride <jim.bride@linux.intel.com>
>
> For DP compliance we need to be able to control the output color
> type for the pipe associated with the DP port. When a specific DP
> compliance test is requested with specific BPC value, we read the BPC
> value from the DPCD register and store it in the intel_dp structure.
> If this BPC value in intel_dp structure  has a non-zero value
> and we're on a display port connector, then we use the value to
> calculate the bpp for the pipe.  For cases where we are
> not on DP or there has not been an overridden value then we behave
> as normal.
>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> 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_display.c |  4 +++-
>  drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 11 +++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b7a7ed8..bb1cca2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12694,11 +12694,13 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  
>  	/* Clamp display bpp to EDID value */
>  	for_each_connector_in_state(state, connector, connector_state, i) {
> +

Unrelated change, and one we don't want.

>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
>  		connected_sink_compute_bpp(to_intel_connector(connector),
> -					   pipe_config);
> +						   pipe_config);
> +

Unrelated change, and one we don't want.

>  	}
>  
>  	return bpp;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6693918..f93e130 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1549,6 +1549,13 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (bpc > 0)
>  		bpp = min(bpp, 3*bpc);
>  
> +	/* For DP Compliance we override the computed bpp for the pipe */
> +	if (intel_dp->compliance_force_bpc != 0) {

You don't actually *set* compliance_force_bpc in this patch, making all
of this a complicated nop. We don't want this kind of changes, because
if this regresses, it'll regress in the patch actually *using* the code,
i.e. the patch setting compliance_force_bpc, not this one.

The rationale is the same as for adding unused functions as prep
patches.

> +		pipe_config->pipe_bpp =	intel_dp->compliance_force_bpc*3;

How about making that compliance_force_bpp and setting it to * 3
directly in one place instead of sprinkling the * 3 all over the place?

> +		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
> +			      pipe_config->pipe_bpp);
> +	}
> +
>  	return bpp;
>  }
>  
> @@ -1629,6 +1636,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	/* Walk through all bpp values. Luckily they're all nicely spaced with 2
>  	 * bpc in between. */
>  	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> +

Unrelated change. Please pay attention to these.

>  	if (is_edp(intel_dp)) {
>  
>  		/* Get bpp from vbt only for panels that dont have bpp in edid */
> @@ -4109,6 +4117,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	intel_dp->compliance_test_data = 0;
>  	intel_dp->compliance_test_lane_count = 0;
>  	intel_dp->compliance_test_link_rate = 0;
> +	intel_dp->compliance_force_bpc = 0;

Again, benefits from having a sub struct.

>  
>  	/*
>  	 * Now read the DPCD to see if it's actually running
> @@ -4434,6 +4443,7 @@ 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_force_bpc = 0;
>  		intel_dp->compliance_test_lane_count = 0;
>  		intel_dp->compliance_test_link_rate = 0;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index b029d10..940f173 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -45,6 +45,14 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->has_pch_encoder = false;
>  	bpp = 24;
> +	/* For DP Compliance we need to ensurethat we can override
> +	 * the computed bpp for the pipe
> +	 */

Unnecessary comment.

> +	if (intel_dp->compliance_force_bpc) {
> +		bpp = intel_dp->compliance_force_bpc * 3;
> +		DRM_DEBUG_KMS("Setting pipe bpp to %d\n",
> +			      bpp);
> +	}
>  	/*
>  	 * for MST we always configure max link bw - the spec doesn't
>  	 * seem to suggest we should do otherwise.
> @@ -52,8 +60,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
>  
>  	pipe_config->lane_count = lane_count;
> -
> -	pipe_config->pipe_bpp = 24;
> +	pipe_config->pipe_bpp = bpp;
>  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>  
>  	state = pipe_config->base.state;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1e88288..3eb428e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -960,6 +960,7 @@ struct intel_dp {
>  	bool compliance_test_active;
>  	u8 compliance_test_lane_count;
>  	u8 compliance_test_link_rate;
> +	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */

unsigned long? How about plain int?

>  };
>  
>  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

[PATCH 4/5] drm: Add definitions for DP compliance Video pattern tests

[Manasi Navare]

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 include/drm/drm_dp_helper.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 55bbeb0..f2c04ec 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -415,7 +415,21 @@
 
 #define DP_TEST_LANE_COUNT		    0x220
 
-#define DP_TEST_PATTERN			    0x221
+#define DP_TEST_PATTERN				0x221
+#define DP_COLOR_RAMP				(1 << 0)
+#define DP_TEST_H_WIDTH				0x22E
+#define DP_TEST_V_HEIGHT			0x230
+#define DP_TEST_MISC				0x232
+#define DP_VIDEO_PATTERN_RGB_FORMAT		0
+#define DP_TEST_COLOR_FORMAT_MASK		0x6
+#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)
+#define DP_VIDEO_PATTERN_VESA			0
+#define DP_TEST_BIT_DEPTH_MASK			0x00E0
+#define DP_TEST_BIT_DEPTH_6			0
+#define DP_TEST_BIT_DEPTH_8			1
+#define DP_TEST_MISC_BIT_1			1
+#define DP_TEST_MISC_BIT_3			3
+#define DP_TEST_MISC_BIT_5			5
 
 #define DP_TEST_CRC_R_CR		    0x240
 #define DP_TEST_CRC_G_Y			    0x242
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Add definitions for DP compliance Video pattern tests

[Jani Nikula]

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  include/drm/drm_dp_helper.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 55bbeb0..f2c04ec 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -415,7 +415,21 @@
>  
>  #define DP_TEST_LANE_COUNT		    0x220
>  
> -#define DP_TEST_PATTERN			    0x221
> +#define DP_TEST_PATTERN				0x221

Unnecessary indentation change. Please observe the code around you. It
may not have the best indentation style, but stick with what's there
for all the other DPCD address definitions.

> +#define DP_COLOR_RAMP				(1 << 0)

See how all the other address *content* definitions have a space between
"#" and "define". I'm not saying I like it, but it's uniform in the
file.

While at it, why not add all of the defines for TEST_PATTERN. And
observe how they are not bit patterns, so (1 << 0) should be just 1.

DP_NO_TEST_PATTERN
DP_COLOR_RAMPS
DP_BLACK_AND_WHITE_VERTICAL_LINES
DP_COLOR_SQUARE

> +#define DP_TEST_H_WIDTH				0x22E

Note that across the file, almost all addrses defines have a blank line
between them, to separate content definitions from other addresses.

> +#define DP_TEST_V_HEIGHT			0x230

I guess I'd do

#define DP_TEST_V_HEIGHT_HI			0x230
#define DP_TEST_V_HEIGHT_LO			0x231

You don't actually have to *use* both definitions if you can write both
in one go, but this saves the trouble of checking the DP spec when it's
documented as #defines here.

> +#define DP_TEST_MISC				0x232
> +#define DP_VIDEO_PATTERN_RGB_FORMAT		0

The convention is to shift the 0 too so it's obvious where it
fits. _MASK goes before the values. Please add all the values.

> +#define DP_TEST_COLOR_FORMAT_MASK		0x6
> +#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)

And the values?

> +#define DP_VIDEO_PATTERN_VESA			0
> +#define DP_TEST_BIT_DEPTH_MASK			0x00E0
> +#define DP_TEST_BIT_DEPTH_6			0

> +#define DP_TEST_BIT_DEPTH_8			1

Just add all of the values at once.

> +#define DP_TEST_MISC_BIT_1			1
> +#define DP_TEST_MISC_BIT_3			3
> +#define DP_TEST_MISC_BIT_5			5
>  
>  #define DP_TEST_CRC_R_CR		    0x240
>  #define DP_TEST_CRC_G_Y			    0x242

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

[PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Manasi Navare]

The intel_dp_autotest_video_pattern() function gets invoked through the
compliance test handler on a HPD short pulse if the test type is
set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
reads to read the requested test pattern, video pattern resolution,
frame rate and bits per color value. The results of this analysis
are handed off to userspace so that the userspace app can set the
video pattern mode appropriately for the test result/response.

The compliance_test_active flag is set at the end of the individual
test handling functions. This is so that the kernel-side operations
can be completed without the risk of interruption from the userspace
app that is polling on that flag.

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/i915_debugfs.c | 14 +++++++-
 drivers/gpu/drm/i915/intel_dp.c     | 68 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
 3 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3799a12..b048a0e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4545,7 +4545,19 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 		if (connector->status == connector_status_connected &&
 		    connector->encoder != NULL) {
 			intel_dp = enc_to_intel_dp(connector->encoder);
-			seq_printf(m, "%lx", intel_dp->compliance_test_data);
+			if (intel_dp->compliance_test_type ==
+			    DP_TEST_LINK_EDID_READ)
+				seq_printf(m, "%lx",
+					   intel_dp->compliance_test_data);
+			else if (intel_dp->compliance_test_type ==
+				 DP_TEST_LINK_VIDEO_PATTERN) {
+				seq_printf(m, "hdisplay: %d\n",
+					   intel_dp->test_data.hdisplay);
+				seq_printf(m, "vdisplay: %d\n",
+					   intel_dp->test_data.vdisplay);
+				seq_printf(m, "bpc: %u\n",
+					   intel_dp->test_data.bpc);
+			}
 		} else
 			seq_puts(m, "0");
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f93e130..784f86e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -28,8 +28,10 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/types.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <asm/byteorder.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -3868,7 +3870,73 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_pattern;
+	uint16_t test_misc;
+	__be16 h_width, v_height;
+	int status = 0;
+
+	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
+				  &test_pattern, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read test pattern from "
+			      "reference sink\n");
+		return 0;
+	}
+	if (test_pattern != DP_COLOR_RAMP)
+		return test_result;
+	intel_dp->test_data.video_pattern = test_pattern;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
+				  &h_width, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read H Width from "
+			      "reference sink\n");
+		return 0;
+	}
+	intel_dp->test_data.hdisplay = be16_to_cpu(h_width);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
+				  &v_height, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read V Height from "
+			      "reference sink\n");
+		return 0;
+	}
+	intel_dp->test_data.vdisplay = be16_to_cpu(v_height);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
+				  &test_misc, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read TEST MISC from "
+			      "reference sink\n");
+		return 0;
+	}
+	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> DP_TEST_MISC_BIT_1) !=
+	    DP_VIDEO_PATTERN_RGB_FORMAT)
+		return test_result;
+	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> DP_TEST_MISC_BIT_3) !=
+	    DP_VIDEO_PATTERN_VESA)
+		return test_result;
+	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> DP_TEST_MISC_BIT_5) {
+	case 0:
+		intel_dp->compliance_force_bpc = 6;
+		intel_dp->test_data.bpc = 6;
+		break;
+	case 1:
+		intel_dp->compliance_force_bpc = 8;
+		intel_dp->test_data.bpc = 8;
+		break;
+	default:
+		return test_result;
+	}
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_test_active = 1;
+
+	test_result = DP_TEST_ACK;
+
 	return test_result;
+
 }
 
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3eb428e..ff4431e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -880,6 +880,12 @@ struct intel_dp_desc {
 	u8 sw_minor_rev;
 } __packed;
 
+struct compliance_test_data {
+	uint8_t video_pattern;
+	uint16_t hdisplay, vdisplay;
+	uint8_t bpc;
+};
+
 struct intel_dp {
 	i915_reg_t output_reg;
 	i915_reg_t aux_ch_ctl_reg;
@@ -961,6 +967,9 @@ struct intel_dp {
 	u8 compliance_test_lane_count;
 	u8 compliance_test_link_rate;
 	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
+	struct compliance_test_data test_data;   /* a structure to hold all dp
+						  * compliance test data
+						  */
 };
 
 struct intel_lspcon {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Daniel Vetter]

On Tue, Nov 22, 2016 at 01:39:26PM -0800, Manasi Navare wrote:
> The intel_dp_autotest_video_pattern() function gets invoked through the
> compliance test handler on a HPD short pulse if the test type is
> set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> reads to read the requested test pattern, video pattern resolution,
> frame rate and bits per color value. The results of this analysis
> are handed off to userspace so that the userspace app can set the
> video pattern mode appropriately for the test result/response.
> 
> The compliance_test_active flag is set at the end of the individual
> test handling functions. This is so that the kernel-side operations
> can be completed without the risk of interruption from the userspace
> app that is polling on that flag.
> 
> 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/i915_debugfs.c | 14 +++++++-
>  drivers/gpu/drm/i915/intel_dp.c     | 68 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
>  3 files changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3799a12..b048a0e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4545,7 +4545,19 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  		if (connector->status == connector_status_connected &&
>  		    connector->encoder != NULL) {
>  			intel_dp = enc_to_intel_dp(connector->encoder);
> -			seq_printf(m, "%lx", intel_dp->compliance_test_data);
> +			if (intel_dp->compliance_test_type ==
> +			    DP_TEST_LINK_EDID_READ)
> +				seq_printf(m, "%lx",
> +					   intel_dp->compliance_test_data);
> +			else if (intel_dp->compliance_test_type ==
> +				 DP_TEST_LINK_VIDEO_PATTERN) {
> +				seq_printf(m, "hdisplay: %d\n",
> +					   intel_dp->test_data.hdisplay);
> +				seq_printf(m, "vdisplay: %d\n",
> +					   intel_dp->test_data.vdisplay);
> +				seq_printf(m, "bpc: %u\n",
> +					   intel_dp->test_data.bpc);
> +			}
>  		} else
>  			seq_puts(m, "0");
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f93e130..784f86e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -28,8 +28,10 @@
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/types.h>
>  #include <linux/notifier.h>
>  #include <linux/reboot.h>
> +#include <asm/byteorder.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> @@ -3868,7 +3870,73 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t test_pattern;
> +	uint16_t test_misc;
> +	__be16 h_width, v_height;
> +	int status = 0;
> +
> +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> +				  &test_pattern, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read test pattern from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	if (test_pattern != DP_COLOR_RAMP)
> +		return test_result;
> +	intel_dp->test_data.video_pattern = test_pattern;
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> +				  &h_width, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read H Width from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	intel_dp->test_data.hdisplay = be16_to_cpu(h_width);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> +				  &v_height, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read V Height from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	intel_dp->test_data.vdisplay = be16_to_cpu(v_height);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> +				  &test_misc, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read TEST MISC from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> DP_TEST_MISC_BIT_1) !=
> +	    DP_VIDEO_PATTERN_RGB_FORMAT)
> +		return test_result;
> +	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> DP_TEST_MISC_BIT_3) !=
> +	    DP_VIDEO_PATTERN_VESA)
> +		return test_result;
> +	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> DP_TEST_MISC_BIT_5) {
> +	case 0:
> +		intel_dp->compliance_force_bpc = 6;
> +		intel_dp->test_data.bpc = 6;
> +		break;
> +	case 1:
> +		intel_dp->compliance_force_bpc = 8;
> +		intel_dp->test_data.bpc = 8;
> +		break;
> +	default:
> +		return test_result;
> +	}
> +	/* Set test active flag here so userspace doesn't interrupt things */
> +	intel_dp->compliance_test_active = 1;
> +
> +	test_result = DP_TEST_ACK;
> +
>  	return test_result;
> +
>  }
>  
>  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3eb428e..ff4431e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -880,6 +880,12 @@ struct intel_dp_desc {
>  	u8 sw_minor_rev;
>  } __packed;
>  
> +struct compliance_test_data {
> +	uint8_t video_pattern;
> +	uint16_t hdisplay, vdisplay;
> +	uint8_t bpc;
> +};
> +
>  struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
> @@ -961,6 +967,9 @@ struct intel_dp {
>  	u8 compliance_test_lane_count;
>  	u8 compliance_test_link_rate;
>  	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> +	struct compliance_test_data test_data;   /* a structure to hold all dp
> +						  * compliance test data
> +						  */

I like this idea of tying all the compliance test data into a sub-struct,
but please refactor the existing code (and the force_bpc you're adding in
this series) to also use that. Consistency wins over pretty imo.

Otherwise seems all reasonable, but this isn't a detailed review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Jani Nikula]

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> The intel_dp_autotest_video_pattern() function gets invoked through the
> compliance test handler on a HPD short pulse if the test type is
> set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> reads to read the requested test pattern, video pattern resolution,
> frame rate and bits per color value. The results of this analysis
> are handed off to userspace so that the userspace app can set the
> video pattern mode appropriately for the test result/response.
>
> The compliance_test_active flag is set at the end of the individual
> test handling functions. This is so that the kernel-side operations
> can be completed without the risk of interruption from the userspace
> app that is polling on that flag.

I've brought this up before, but I think for this stuff the way to go is
to have the userspace read the DPCD directly. We have the dev node for
it.

With the approach in this patch, we'll just end up reading a bunch of
stuff from DPCD in kernel, doing error handling for that, decoding and
sanity checking the values, putting them in debugfs for the userspace to
read, having userspace code read debugfs, doing error handling for that,
decoding and sanity checking the data, finally doing something based on
the data.

You'll also get a *much* faster turnaround for getting your userspace
code done than getting all of this in kernel first, then tweaking your
userspace, having to update both of those in lockstep, etc. When this is
based on reading DPCD directly, you can just add new stuff quickly in
userspace, with no kernel dependency.

The easiest way would be to have an indication in debugfs for userspace
that there's something interesting in DPCD. Just a simple thing.

BR,
Jani.


>
> 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/i915_debugfs.c | 14 +++++++-
>  drivers/gpu/drm/i915/intel_dp.c     | 68 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
>  3 files changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3799a12..b048a0e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4545,7 +4545,19 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  		if (connector->status == connector_status_connected &&
>  		    connector->encoder != NULL) {
>  			intel_dp = enc_to_intel_dp(connector->encoder);
> -			seq_printf(m, "%lx", intel_dp->compliance_test_data);
> +			if (intel_dp->compliance_test_type ==
> +			    DP_TEST_LINK_EDID_READ)
> +				seq_printf(m, "%lx",
> +					   intel_dp->compliance_test_data);
> +			else if (intel_dp->compliance_test_type ==
> +				 DP_TEST_LINK_VIDEO_PATTERN) {
> +				seq_printf(m, "hdisplay: %d\n",
> +					   intel_dp->test_data.hdisplay);
> +				seq_printf(m, "vdisplay: %d\n",
> +					   intel_dp->test_data.vdisplay);
> +				seq_printf(m, "bpc: %u\n",
> +					   intel_dp->test_data.bpc);
> +			}
>  		} else
>  			seq_puts(m, "0");
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f93e130..784f86e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -28,8 +28,10 @@
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/types.h>
>  #include <linux/notifier.h>
>  #include <linux/reboot.h>
> +#include <asm/byteorder.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> @@ -3868,7 +3870,73 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t test_pattern;
> +	uint16_t test_misc;
> +	__be16 h_width, v_height;
> +	int status = 0;
> +
> +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> +				  &test_pattern, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read test pattern from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	if (test_pattern != DP_COLOR_RAMP)
> +		return test_result;
> +	intel_dp->test_data.video_pattern = test_pattern;
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> +				  &h_width, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read H Width from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	intel_dp->test_data.hdisplay = be16_to_cpu(h_width);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> +				  &v_height, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read V Height from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	intel_dp->test_data.vdisplay = be16_to_cpu(v_height);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> +				  &test_misc, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read TEST MISC from "
> +			      "reference sink\n");
> +		return 0;
> +	}
> +	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> DP_TEST_MISC_BIT_1) !=
> +	    DP_VIDEO_PATTERN_RGB_FORMAT)
> +		return test_result;
> +	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> DP_TEST_MISC_BIT_3) !=
> +	    DP_VIDEO_PATTERN_VESA)
> +		return test_result;
> +	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> DP_TEST_MISC_BIT_5) {
> +	case 0:
> +		intel_dp->compliance_force_bpc = 6;
> +		intel_dp->test_data.bpc = 6;
> +		break;
> +	case 1:
> +		intel_dp->compliance_force_bpc = 8;
> +		intel_dp->test_data.bpc = 8;
> +		break;
> +	default:
> +		return test_result;
> +	}
> +	/* Set test active flag here so userspace doesn't interrupt things */
> +	intel_dp->compliance_test_active = 1;
> +
> +	test_result = DP_TEST_ACK;
> +
>  	return test_result;
> +
>  }
>  
>  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3eb428e..ff4431e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -880,6 +880,12 @@ struct intel_dp_desc {
>  	u8 sw_minor_rev;
>  } __packed;
>  
> +struct compliance_test_data {
> +	uint8_t video_pattern;
> +	uint16_t hdisplay, vdisplay;
> +	uint8_t bpc;
> +};
> +
>  struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
> @@ -961,6 +967,9 @@ struct intel_dp {
>  	u8 compliance_test_lane_count;
>  	u8 compliance_test_link_rate;
>  	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> +	struct compliance_test_data test_data;   /* a structure to hold all dp
> +						  * compliance test data
> +						  */
>  };
>  
>  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

Re: [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Ville Syrjälä]

On Wed, Nov 23, 2016 at 03:37:24PM +0200, Jani Nikula wrote:
> On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > The intel_dp_autotest_video_pattern() function gets invoked through the
> > compliance test handler on a HPD short pulse if the test type is
> > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> > reads to read the requested test pattern, video pattern resolution,
> > frame rate and bits per color value. The results of this analysis
> > are handed off to userspace so that the userspace app can set the
> > video pattern mode appropriately for the test result/response.
> >
> > The compliance_test_active flag is set at the end of the individual
> > test handling functions. This is so that the kernel-side operations
> > can be completed without the risk of interruption from the userspace
> > app that is polling on that flag.
> 
> I've brought this up before, but I think for this stuff the way to go is
> to have the userspace read the DPCD directly. We have the dev node for
> it.
> 
> With the approach in this patch, we'll just end up reading a bunch of
> stuff from DPCD in kernel, doing error handling for that, decoding and
> sanity checking the values, putting them in debugfs for the userspace to
> read, having userspace code read debugfs, doing error handling for that,
> decoding and sanity checking the data, finally doing something based on
> the data.
> 
> You'll also get a *much* faster turnaround for getting your userspace
> code done than getting all of this in kernel first, then tweaking your
> userspace, having to update both of those in lockstep, etc. When this is
> based on reading DPCD directly, you can just add new stuff quickly in
> userspace, with no kernel dependency.
> 
> The easiest way would be to have an indication in debugfs for userspace
> that there's something interesting in DPCD. Just a simple thing.

Or just have the kernel fire off an uevent...

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Jani Nikula]

On Wed, 23 Nov 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Nov 23, 2016 at 03:37:24PM +0200, Jani Nikula wrote:
>> On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > The intel_dp_autotest_video_pattern() function gets invoked through the
>> > compliance test handler on a HPD short pulse if the test type is
>> > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
>> > reads to read the requested test pattern, video pattern resolution,
>> > frame rate and bits per color value. The results of this analysis
>> > are handed off to userspace so that the userspace app can set the
>> > video pattern mode appropriately for the test result/response.
>> >
>> > The compliance_test_active flag is set at the end of the individual
>> > test handling functions. This is so that the kernel-side operations
>> > can be completed without the risk of interruption from the userspace
>> > app that is polling on that flag.
>> 
>> I've brought this up before, but I think for this stuff the way to go is
>> to have the userspace read the DPCD directly. We have the dev node for
>> it.
>> 
>> With the approach in this patch, we'll just end up reading a bunch of
>> stuff from DPCD in kernel, doing error handling for that, decoding and
>> sanity checking the values, putting them in debugfs for the userspace to
>> read, having userspace code read debugfs, doing error handling for that,
>> decoding and sanity checking the data, finally doing something based on
>> the data.
>> 
>> You'll also get a *much* faster turnaround for getting your userspace
>> code done than getting all of this in kernel first, then tweaking your
>> userspace, having to update both of those in lockstep, etc. When this is
>> based on reading DPCD directly, you can just add new stuff quickly in
>> userspace, with no kernel dependency.
>> 
>> The easiest way would be to have an indication in debugfs for userspace
>> that there's something interesting in DPCD. Just a simple thing.
>
> Or just have the kernel fire off an uevent...

I know, I know, I had that written in my mail already, but swallowed it
not to add another dependency on another new UABI... we can do that
later.

BR,
Jani.


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

Re: [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Daniel Vetter]

On Wed, Nov 23, 2016 at 03:58:34PM +0200, Jani Nikula wrote:
> On Wed, 23 Nov 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Nov 23, 2016 at 03:37:24PM +0200, Jani Nikula wrote:
> >> On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >> > The intel_dp_autotest_video_pattern() function gets invoked through the
> >> > compliance test handler on a HPD short pulse if the test type is
> >> > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> >> > reads to read the requested test pattern, video pattern resolution,
> >> > frame rate and bits per color value. The results of this analysis
> >> > are handed off to userspace so that the userspace app can set the
> >> > video pattern mode appropriately for the test result/response.
> >> >
> >> > The compliance_test_active flag is set at the end of the individual
> >> > test handling functions. This is so that the kernel-side operations
> >> > can be completed without the risk of interruption from the userspace
> >> > app that is polling on that flag.
> >> 
> >> I've brought this up before, but I think for this stuff the way to go is
> >> to have the userspace read the DPCD directly. We have the dev node for
> >> it.
> >> 
> >> With the approach in this patch, we'll just end up reading a bunch of
> >> stuff from DPCD in kernel, doing error handling for that, decoding and
> >> sanity checking the values, putting them in debugfs for the userspace to
> >> read, having userspace code read debugfs, doing error handling for that,
> >> decoding and sanity checking the data, finally doing something based on
> >> the data.
> >> 
> >> You'll also get a *much* faster turnaround for getting your userspace
> >> code done than getting all of this in kernel first, then tweaking your
> >> userspace, having to update both of those in lockstep, etc. When this is
> >> based on reading DPCD directly, you can just add new stuff quickly in
> >> userspace, with no kernel dependency.
> >> 
> >> The easiest way would be to have an indication in debugfs for userspace
> >> that there's something interesting in DPCD. Just a simple thing.
> >
> > Or just have the kernel fire off an uevent...
> 
> I know, I know, I had that written in my mail already, but swallowed it
> not to add another dependency on another new UABI... we can do that
> later.

In principle I'd agree, but the trouble with dp compliance is that the CTS
expects us to do stupid shit sometimes, like that test pattern forced as
6bpc. So if we decode&handle everything in userspace then the question is
how to feed all those special cases back into the kernel, which means more
debugfs interfaces. Just going in the other direction.

Given that it's a bit a toss-up unfortunately, and the current approach
seems reasonable.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Add support for DP Video pattern compliance tests

[Ville Syrjälä]

On Wed, Nov 23, 2016 at 03:17:53PM +0100, Daniel Vetter wrote:
> On Wed, Nov 23, 2016 at 03:58:34PM +0200, Jani Nikula wrote:
> > On Wed, 23 Nov 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > On Wed, Nov 23, 2016 at 03:37:24PM +0200, Jani Nikula wrote:
> > >> On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > >> > The intel_dp_autotest_video_pattern() function gets invoked through the
> > >> > compliance test handler on a HPD short pulse if the test type is
> > >> > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> > >> > reads to read the requested test pattern, video pattern resolution,
> > >> > frame rate and bits per color value. The results of this analysis
> > >> > are handed off to userspace so that the userspace app can set the
> > >> > video pattern mode appropriately for the test result/response.
> > >> >
> > >> > The compliance_test_active flag is set at the end of the individual
> > >> > test handling functions. This is so that the kernel-side operations
> > >> > can be completed without the risk of interruption from the userspace
> > >> > app that is polling on that flag.
> > >> 
> > >> I've brought this up before, but I think for this stuff the way to go is
> > >> to have the userspace read the DPCD directly. We have the dev node for
> > >> it.
> > >> 
> > >> With the approach in this patch, we'll just end up reading a bunch of
> > >> stuff from DPCD in kernel, doing error handling for that, decoding and
> > >> sanity checking the values, putting them in debugfs for the userspace to
> > >> read, having userspace code read debugfs, doing error handling for that,
> > >> decoding and sanity checking the data, finally doing something based on
> > >> the data.
> > >> 
> > >> You'll also get a *much* faster turnaround for getting your userspace
> > >> code done than getting all of this in kernel first, then tweaking your
> > >> userspace, having to update both of those in lockstep, etc. When this is
> > >> based on reading DPCD directly, you can just add new stuff quickly in
> > >> userspace, with no kernel dependency.
> > >> 
> > >> The easiest way would be to have an indication in debugfs for userspace
> > >> that there's something interesting in DPCD. Just a simple thing.
> > >
> > > Or just have the kernel fire off an uevent...
> > 
> > I know, I know, I had that written in my mail already, but swallowed it
> > not to add another dependency on another new UABI... we can do that
> > later.
> 
> In principle I'd agree, but the trouble with dp compliance is that the CTS
> expects us to do stupid shit sometimes, like that test pattern forced as
> 6bpc. So if we decode&handle everything in userspace then the question is
> how to feed all those special cases back into the kernel, which means more
> debugfs interfaces. Just going in the other direction.
> 
> Given that it's a bit a toss-up unfortunately, and the current approach
> seems reasonable.

The kernel could of course parse whatever is needed from the test
request as well, but leave it up to userspace to initiate the actual
test and ack/nack the test request. So I don't see a real need for
a debugfs interface in either direction.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx