[PATCH 1/2] drm: dp helper: Add DP test sink CRC definition.

[PATCH 1/2] drm: dp helper: Add DP test sink CRC definition.

[Rodrigo Vivi]

This address will be used to verify panel CRC for test and
validation purposes.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 include/drm/drm_dp_helper.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1d09050..ba0b90d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -279,11 +279,21 @@
 
 #define DP_TEST_PATTERN			    0x221
 
+#define DP_TEST_CRC_R_CR		    0x240
+#define DP_TEST_CRC_G_Y			    0x242
+#define DP_TEST_CRC_B_CB		    0x244
+
+#define DP_TEST_SINK_MISC		    0x246
+#define DP_TEST_CRC_SUPPORTED 		    (1 << 5)
+
 #define DP_TEST_RESPONSE		    0x260
 # define DP_TEST_ACK			    (1 << 0)
 # define DP_TEST_NAK			    (1 << 1)
 # define DP_TEST_EDID_CHECKSUM_WRITE	    (1 << 2)
 
+#define DP_TEST_SINK			    0x270
+#define DP_TEST_SINK_START    		    (1 << 0)
+
 #define DP_SOURCE_OUI			    0x300
 #define DP_SINK_OUI			    0x400
 #define DP_BRANCH_OUI			    0x500
-- 
1.8.3.1

[PATCH 2/2] drm/i915: debugs: Add support for probing DP sink CRC.

[Rodrigo Vivi]

This debugfs interface will allow intel-gpu-tools test case
to verify if screen has been updated properly on cases like PSR.

v2: Accepted all Daniel's suggestions:
    * grab modeset lock
    * loop over connector and check DPMS on
    * return errors
    * use _eDP1 suffix for easy future extension
    * don't cache crc_supported neither latest crc
    * return crc as a full array and read it at once with aux.
    * use 0 to turn TEST_SINK off.
    * split the drm_helpers definitions in another patch.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 30 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 3 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 75a489e..36424ca 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1876,6 +1876,45 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_sink_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct intel_dp *intel_dp = NULL;
+	int ret;
+	u8 crc[6];
+
+	drm_modeset_lock_all(dev);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
+		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		ret = intel_dp_sink_crc(intel_dp, crc);
+		if (ret) {
+			drm_modeset_unlock_all(dev);
+			return ret;
+		}
+
+		seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n",
+			   crc[0], crc[1], crc[2],
+			   crc[3], crc[4], crc[5]);
+		drm_modeset_unlock_all(dev);
+		return 0;
+	}
+	drm_modeset_unlock_all(dev);
+	return -EAGAIN;
+}
+
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -3232,6 +3271,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7df5085..deedcf2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2846,6 +2846,36 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 }
 
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
+	u8 buf[1];
+
+	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf,
+					    1))
+		return -EAGAIN;
+
+	if (!buf[0] & DP_TEST_CRC_SUPPORTED)
+		return -ENOTTY;
+
+	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
+					 DP_TEST_SINK_START))
+		return -EAGAIN;
+
+	/* Wait 2 vblanks to be sure we will have the correct CRC value */
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
+		return -EAGAIN;
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
+	return 0;
+}
+
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..fe7afe7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -719,6 +719,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_config *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
-- 
1.8.3.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915: debugs: Add support for probing DP sink CRC.

[Damien Lespiau]

On Tue, Jan 14, 2014 at 04:21:50PM -0200, Rodrigo Vivi wrote:
> This debugfs interface will allow intel-gpu-tools test case
> to verify if screen has been updated properly on cases like PSR.
> 
> v2: Accepted all Daniel's suggestions:
>     * grab modeset lock
>     * loop over connector and check DPMS on
>     * return errors
>     * use _eDP1 suffix for easy future extension
>     * don't cache crc_supported neither latest crc
>     * return crc as a full array and read it at once with aux.
>     * use 0 to turn TEST_SINK off.
>     * split the drm_helpers definitions in another patch.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 30 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  3 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 75a489e..36424ca 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1876,6 +1876,45 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static int i915_sink_crc(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
> +	struct intel_dp *intel_dp = NULL;
> +	int ret;
> +	u8 crc[6];
> +
> +	drm_modeset_lock_all(dev);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->base.dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		if (encoder->type != INTEL_OUTPUT_EDP)
> +			continue;
> +
> +		intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +		ret = intel_dp_sink_crc(intel_dp, crc);
> +		if (ret) {
> +			drm_modeset_unlock_all(dev);
> +			return ret;
> +		}
> +
> +		seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n",
> +			   crc[0], crc[1], crc[2],
> +			   crc[3], crc[4], crc[5]);

Isn't the h modifiers for shorts? also the h or hh modifiers are not
really useful, shorts and chars are promoted to ints in varargs
functions anyway.

> +		drm_modeset_unlock_all(dev);
> +		return 0;
> +	}
> +	drm_modeset_unlock_all(dev);
> +	return -EAGAIN;

EAGAIN has the meaning of "no data available yet, try again later".
Maybe an -ENODEV would be more appropriate in that case (no eDP device).

> +}
> +
>  static int i915_energy_uJ(struct seq_file *m, void *data)
>  {
>  	struct drm_info_node *node = m->private;
> @@ -3232,6 +3271,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_dpio", i915_dpio_info, 0},
>  	{"i915_llc", i915_llc, 0},
>  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
> +	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
>  	{"i915_energy_uJ", i915_energy_uJ, 0},
>  	{"i915_pc8_status", i915_pc8_status, 0},
>  	{"i915_power_domain_info", i915_power_domain_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7df5085..deedcf2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2846,6 +2846,36 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  	ironlake_edp_panel_vdd_off(intel_dp, false);
>  }
>  
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(intel_dig_port->base.base.crtc);
> +	u8 buf[1];
> +
> +	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf,
> +					    1))
> +		return -EAGAIN;
> +
> +	if (!buf[0] & DP_TEST_CRC_SUPPORTED)
> +		return -ENOTTY;
> +
> +	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
> +					 DP_TEST_SINK_START))
> +		return -EAGAIN;
> +
> +	/* Wait 2 vblanks to be sure we will have the correct CRC value */
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);

I think there's a better way to do this. There's a TEST_CRC_COUNT in
TEST_SINK_MISC that is incremented everytime the CRCs are updated. You
could:
 * start by grabbing TEST_CRC_COUNT
 * have a loop that waits for a vblank, check if the TEST_CRC_COUNT has
   changed
 * return the new CRC if the update has occured
 * or give up after a number of waits (say 10)

> +
> +	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
> +		return -EAGAIN;
> +
> +	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
> +	return 0;
> +}
> +
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..fe7afe7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -719,6 +719,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>  void intel_dp_check_link_status(struct intel_dp *intel_dp);
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_config *pipe_config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: debugs: Add support for probing DP sink CRC.

[Damien Lespiau]

On Thu, Jan 16, 2014 at 05:55:06PM +0000, Damien Lespiau wrote:
> > +	/* Wait 2 vblanks to be sure we will have the correct CRC value */
> > +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> > +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> 
> I think there's a better way to do this. There's a TEST_CRC_COUNT in
> TEST_SINK_MISC that is incremented everytime the CRCs are updated. You
> could:
>  * start by grabbing TEST_CRC_COUNT
>  * have a loop that waits for a vblank, check if the TEST_CRC_COUNT has
>    changed
>  * return the new CRC if the update has occured
>  * or give up after a number of waits (say 10)
 
There seem to be support for an interrupt to come from the device to
signal the CRCs are ready (AUTOMATED_TEST_REQUEST of
DEVICE_SERVICE_IRQ_VECTOR), but that's pushing it :)

-- 
Damien

[PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Rodrigo Vivi]

This debugfs interface will allow intel-gpu-tools test case
to verify if screen has been updated properly on cases like PSR.

v2: Accepted all Daniel's suggestions:
    * grab modeset lock
    * loop over connector and check DPMS on
    * return errors
    * use _eDP1 suffix for easy future extension
    * don't cache crc_supported neither latest crc
    * return crc as a full array and read it at once with aux.
    * use 0 to turn TEST_SINK off.
    * split the drm_helpers definitions in another patch.

v3: Accepted 2 Damien's suggestion: remove h from printf hexa
    and return ENODEV when eDP not present instead of EAGAIN.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 30 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 3 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 74866bf..a1d29d1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1922,6 +1922,45 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_sink_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct intel_dp *intel_dp = NULL;
+	int ret;
+	u8 crc[6];
+
+	drm_modeset_lock_all(dev);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
+		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		ret = intel_dp_sink_crc(intel_dp, crc);
+		if (ret) {
+			drm_modeset_unlock_all(dev);
+			return ret;
+		}
+
+		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
+			   crc[0], crc[1], crc[2],
+			   crc[3], crc[4], crc[5]);
+		drm_modeset_unlock_all(dev);
+		return 0;
+	}
+	drm_modeset_unlock_all(dev);
+	return -ENODEV;
+}
+
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -3278,6 +3317,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7df5085..dc646ac 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2846,6 +2846,36 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 }
 
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
+	u8 buf[1];
+
+	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf,
+					    1))
+		return -EAGAIN;
+
+	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
+		return -ENOTTY;
+
+	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
+					 DP_TEST_SINK_START))
+		return -EAGAIN;
+
+	/* Wait 2 vblanks to be sure we will have the correct CRC value */
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
+		return -EAGAIN;
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
+	return 0;
+}
+
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9841f78..b19a43d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -723,6 +723,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_config *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
-- 
1.8.1.2

Re: [PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Jani Nikula]

On Thu, 23 Jan 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> This debugfs interface will allow intel-gpu-tools test case
> to verify if screen has been updated properly on cases like PSR.
>
> v2: Accepted all Daniel's suggestions:
>     * grab modeset lock
>     * loop over connector and check DPMS on
>     * return errors
>     * use _eDP1 suffix for easy future extension
>     * don't cache crc_supported neither latest crc
>     * return crc as a full array and read it at once with aux.
>     * use 0 to turn TEST_SINK off.
>     * split the drm_helpers definitions in another patch.
>
> v3: Accepted 2 Damien's suggestion: remove h from printf hexa
>     and return ENODEV when eDP not present instead of EAGAIN.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 30 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  3 files changed, 71 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 74866bf..a1d29d1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1922,6 +1922,45 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static int i915_sink_crc(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
> +	struct intel_dp *intel_dp = NULL;
> +	int ret;
> +	u8 crc[6];
> +
> +	drm_modeset_lock_all(dev);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->base.dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		if (encoder->type != INTEL_OUTPUT_EDP)
> +			continue;
> +
> +		intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +		ret = intel_dp_sink_crc(intel_dp, crc);
> +		if (ret) {
> +			drm_modeset_unlock_all(dev);
> +			return ret;
> +		}
> +
> +		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> +			   crc[0], crc[1], crc[2],
> +			   crc[3], crc[4], crc[5]);
> +		drm_modeset_unlock_all(dev);
> +		return 0;
> +	}
> +	drm_modeset_unlock_all(dev);
> +	return -ENODEV;

Hi Rodrigo, I really don't like sprinkling the drm_modeset_unlock_all()
on all paths. Please have one return path with the unlock and return
ret.

> +}
> +
>  static int i915_energy_uJ(struct seq_file *m, void *data)
>  {
>  	struct drm_info_node *node = m->private;
> @@ -3278,6 +3317,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_dpio", i915_dpio_info, 0},
>  	{"i915_llc", i915_llc, 0},
>  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
> +	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
>  	{"i915_energy_uJ", i915_energy_uJ, 0},
>  	{"i915_pc8_status", i915_pc8_status, 0},
>  	{"i915_power_domain_info", i915_power_domain_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7df5085..dc646ac 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2846,6 +2846,36 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  	ironlake_edp_panel_vdd_off(intel_dp, false);
>  }
>  
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(intel_dig_port->base.base.crtc);
> +	u8 buf[1];
> +
> +	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf,
> +					    1))

The _retry variant is for when the sink might be in sleep. Is that the
case here?

> +		return -EAGAIN;
> +
> +	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
> +		return -ENOTTY;
> +
> +	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
> +					 DP_TEST_SINK_START))
> +		return -EAGAIN;
> +
> +	/* Wait 2 vblanks to be sure we will have the correct CRC value */
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
> +		return -EAGAIN;

_retry variant is definitely not needed here.

I wonder whether we should try to end test mode in the failure path.

BR,
Jani.


> +
> +	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
> +	return 0;
> +}
> +
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9841f78..b19a43d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -723,6 +723,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>  void intel_dp_check_link_status(struct intel_dp *intel_dp);
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_config *pipe_config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> -- 
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Rodrigo Vivi]

This debugfs interface will allow intel-gpu-tools test case
to verify if screen has been updated properly on cases like PSR.

v2: Accepted all Daniel's suggestions:
    * grab modeset lock
    * loop over connector and check DPMS on
    * return errors
    * use _eDP1 suffix for easy future extension
    * don't cache crc_supported neither latest crc
    * return crc as a full array and read it at once with aux.
    * use 0 to turn TEST_SINK off.
    * split the drm_helpers definitions in another patch.

v3: Accepted 2 Damien's suggestion: remove h from printf hexa
    and return ENODEV when eDP not present instead of EAGAIN.

v4: Accepted 2 Jani' s suggestion: 1 path for unlock and remove
    _retry from aux read.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 39 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 29 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 3 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 74866bf..5f9dba9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1922,6 +1922,44 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_sink_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct intel_dp *intel_dp = NULL;
+	int ret;
+	u8 crc[6];
+
+	drm_modeset_lock_all(dev);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
+		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		ret = intel_dp_sink_crc(intel_dp, crc);
+		if (ret)
+			goto out;
+
+		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
+			   crc[0], crc[1], crc[2],
+			   crc[3], crc[4], crc[5]);
+		goto out;
+	}
+	ret = -ENODEV;
+out:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -3278,6 +3316,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7df5085..a77a623 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2846,6 +2846,35 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 }
 
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
+	u8 buf[1];
+
+	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
+		return -EAGAIN;
+
+	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
+		return -ENOTTY;
+
+	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
+					 DP_TEST_SINK_START))
+		return -EAGAIN;
+
+	/* Wait 2 vblanks to be sure we will have the correct CRC value */
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
+		return -EAGAIN;
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
+	return 0;
+}
+
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9841f78..b19a43d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -723,6 +723,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_config *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
-- 
1.8.1.2

Re: [PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Damien Lespiau]

On Fri, Jan 24, 2014 at 10:05:32AM -0200, Rodrigo Vivi wrote:
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(intel_dig_port->base.base.crtc);
> +	u8 buf[1];
> +
> +	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
> +		return -EAGAIN;
> +
> +	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
> +		return -ENOTTY;
> +
> +	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
> +					 DP_TEST_SINK_START))
> +		return -EAGAIN;
> +
> +	/* Wait 2 vblanks to be sure we will have the correct CRC value */
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +	if (!intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
> +		return -EAGAIN;

There's still one retry hidden here.

-- 
Damien

[PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Rodrigo Vivi]

This debugfs interface will allow intel-gpu-tools test case
to verify if screen has been updated properly on cases like PSR.

v2: Accepted all Daniel's suggestions:
    * grab modeset lock
    * loop over connector and check DPMS on
    * return errors
    * use _eDP1 suffix for easy future extension
    * don't cache crc_supported neither latest crc
    * return crc as a full array and read it at once with aux.
    * use 0 to turn TEST_SINK off.
    * split the drm_helpers definitions in another patch.

v3: Accepted 2 Damien's suggestion: remove h from printf hexa
    and return ENODEV when eDP not present instead of EAGAIN.

v4: Accepted 2 Jani' s suggestion: 1 path for unlock and remove
    _retry from aux read.

v5: removing last missing useless _retry (by Damien)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 39 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 29 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 3 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 74866bf..5f9dba9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1922,6 +1922,44 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_sink_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct intel_dp *intel_dp = NULL;
+	int ret;
+	u8 crc[6];
+
+	drm_modeset_lock_all(dev);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
+		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		ret = intel_dp_sink_crc(intel_dp, crc);
+		if (ret)
+			goto out;
+
+		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
+			   crc[0], crc[1], crc[2],
+			   crc[3], crc[4], crc[5]);
+		goto out;
+	}
+	ret = -ENODEV;
+out:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -3278,6 +3316,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7df5085..34d0e81 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2846,6 +2846,35 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 }
 
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
+	u8 buf[1];
+
+	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
+		return -EAGAIN;
+
+	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
+		return -ENOTTY;
+
+	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
+					 DP_TEST_SINK_START))
+		return -EAGAIN;
+
+	/* Wait 2 vblanks to be sure we will have the correct CRC value */
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
+		return -EAGAIN;
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
+	return 0;
+}
+
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9841f78..b19a43d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -723,6 +723,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_config *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
-- 
1.8.1.2

Re: [PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Damien Lespiau]

On Fri, Jan 24, 2014 at 01:36:17PM -0200, Rodrigo Vivi wrote:
> This debugfs interface will allow intel-gpu-tools test case
> to verify if screen has been updated properly on cases like PSR.
> 
> v2: Accepted all Daniel's suggestions:
>     * grab modeset lock
>     * loop over connector and check DPMS on
>     * return errors
>     * use _eDP1 suffix for easy future extension
>     * don't cache crc_supported neither latest crc
>     * return crc as a full array and read it at once with aux.
>     * use 0 to turn TEST_SINK off.
>     * split the drm_helpers definitions in another patch.
> 
> v3: Accepted 2 Damien's suggestion: remove h from printf hexa
>     and return ENODEV when eDP not present instead of EAGAIN.
> 
> v4: Accepted 2 Jani' s suggestion: 1 path for unlock and remove
>     _retry from aux read.
> 
> v5: removing last missing useless _retry (by Damien)
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>


Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Daniel Vetter]

On Fri, Jan 24, 2014 at 03:43:58PM +0000, Damien Lespiau wrote:
> On Fri, Jan 24, 2014 at 01:36:17PM -0200, Rodrigo Vivi wrote:
> > This debugfs interface will allow intel-gpu-tools test case
> > to verify if screen has been updated properly on cases like PSR.
> > 
> > v2: Accepted all Daniel's suggestions:
> >     * grab modeset lock
> >     * loop over connector and check DPMS on
> >     * return errors
> >     * use _eDP1 suffix for easy future extension
> >     * don't cache crc_supported neither latest crc
> >     * return crc as a full array and read it at once with aux.
> >     * use 0 to turn TEST_SINK off.
> >     * split the drm_helpers definitions in another patch.
> > 
> > v3: Accepted 2 Damien's suggestion: remove h from printf hexa
> >     and return ENODEV when eDP not present instead of EAGAIN.
> > 
> > v4: Accepted 2 Jani' s suggestion: 1 path for unlock and remove
> >     _retry from aux read.
> > 
> > v5: removing last missing useless _retry (by Damien)
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/2] drm: dp helper: Add DP test sink CRC definition.

[Daniel Vetter]

On Tue, Jan 14, 2014 at 04:21:49PM -0200, Rodrigo Vivi wrote:
> This address will be used to verify panel CRC for test and
> validation purposes.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

checkpatch noticed some whitespace fail in here (spaces before tabs). I've
fixed it up.
-Daniel

> ---
>  include/drm/drm_dp_helper.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 1d09050..ba0b90d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -279,11 +279,21 @@
>  
>  #define DP_TEST_PATTERN			    0x221
>  
> +#define DP_TEST_CRC_R_CR		    0x240
> +#define DP_TEST_CRC_G_Y			    0x242
> +#define DP_TEST_CRC_B_CB		    0x244
> +
> +#define DP_TEST_SINK_MISC		    0x246
> +#define DP_TEST_CRC_SUPPORTED 		    (1 << 5)
> +
>  #define DP_TEST_RESPONSE		    0x260
>  # define DP_TEST_ACK			    (1 << 0)
>  # define DP_TEST_NAK			    (1 << 1)
>  # define DP_TEST_EDID_CHECKSUM_WRITE	    (1 << 2)
>  
> +#define DP_TEST_SINK			    0x270
> +#define DP_TEST_SINK_START    		    (1 << 0)
> +
>  #define DP_SOURCE_OUI			    0x300
>  #define DP_SINK_OUI			    0x400
>  #define DP_BRANCH_OUI			    0x500
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Rodrigo Vivi]

This debugfs interface will allow intel-gpu-tools test case
to verify if screen has been updated properly on cases like PSR.

Since the current target is PSR we will provide only the CRC check
for eDP panels. We can latter extend it to all available DP panels.
---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  9 +++++++++
 include/drm/drm_dp_helper.h         | 10 ++++++++++
 4 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 75a489e..0facff1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1876,6 +1876,29 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_sink_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+
+			intel_dp_sink_crc(intel_dp);
+			seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n",
+				   intel_dp->sink_crc.r_cr[0],
+				   intel_dp->sink_crc.r_cr[1],
+				   intel_dp->sink_crc.g_y[0],
+				   intel_dp->sink_crc.g_y[1],
+				   intel_dp->sink_crc.b_cb[0],
+				   intel_dp->sink_crc.b_cb[1]);
+		}
+	return 0;
+}
+
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -3232,6 +3255,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_sink_crc", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7df5085..9933327 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2786,6 +2786,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
+	u8 buf[1];
 
 	if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
 					   sizeof(intel_dp->dpcd)) == 0)
@@ -2810,6 +2811,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		}
 	}
 
+	intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf, 1);
+	intel_dp->sink_crc.supported = buf[0] & DP_TEST_CRC_SUPPORTED;
+
 	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
 	      DP_DWN_STRM_PORT_PRESENT))
 		return true; /* native DP sink */
@@ -2846,6 +2850,33 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 }
 
+void intel_dp_sink_crc(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
+
+	if (!intel_dp->sink_crc.supported)
+		return;
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, DP_TEST_SINK_START);
+
+	/* Wait 2 vblanks to be sure we will have the correct CRC value */
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR,
+				       intel_dp->sink_crc.r_cr, 2);
+	intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_G_Y,
+				       intel_dp->sink_crc.g_y, 2);
+	intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_B_CB,
+				       intel_dp->sink_crc.b_cb, 2);
+
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
+				    ~DP_TEST_SINK_START);
+}
+
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..48533c0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -462,6 +462,13 @@ struct intel_hdmi {
 
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+struct sink_crc {
+	bool supported;
+	u8 r_cr[2];
+	u8 g_y[2];
+	u8 b_cb[2];
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -487,6 +494,7 @@ struct intel_dp {
 	bool want_panel_vdd;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
+	struct sink_crc sink_crc;
 };
 
 struct intel_digital_port {
@@ -719,6 +727,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
+void intel_dp_sink_crc(struct intel_dp *intel_dp);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_config *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1d09050..ba0b90d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -279,11 +279,21 @@
 
 #define DP_TEST_PATTERN			    0x221
 
+#define DP_TEST_CRC_R_CR		    0x240
+#define DP_TEST_CRC_G_Y			    0x242
+#define DP_TEST_CRC_B_CB		    0x244
+
+#define DP_TEST_SINK_MISC		    0x246
+#define DP_TEST_CRC_SUPPORTED 		    (1 << 5)
+
 #define DP_TEST_RESPONSE		    0x260
 # define DP_TEST_ACK			    (1 << 0)
 # define DP_TEST_NAK			    (1 << 1)
 # define DP_TEST_EDID_CHECKSUM_WRITE	    (1 << 2)
 
+#define DP_TEST_SINK			    0x270
+#define DP_TEST_SINK_START    		    (1 << 0)
+
 #define DP_SOURCE_OUI			    0x300
 #define DP_SINK_OUI			    0x400
 #define DP_BRANCH_OUI			    0x500
-- 
1.8.3.1

Re: [PATCH] drm/i915: debugfs: Add support for probing DP sink CRC.

[Daniel Vetter]

Yay, I'm really happy that after fbc testcases for psr are now also
shaping up. Some small stuff below.
-Daniel

On Thu, Jan 9, 2014 at 8:47 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> This debugfs interface will allow intel-gpu-tools test case
> to verify if screen has been updated properly on cases like PSR.
>
> Since the current target is PSR we will provide only the CRC check
> for eDP panels. We can latter extend it to all available DP panels.

Sob-line?

Also I think it'd be nice to have a very basic test just to exercise
this code. Probably simplest would be to extend Damien's basic crc
testcase:
- If there's no edp connector, then skip it.
- Skip when the debugfs file isn't there.
- Skip if the panel doesn't support CRC (see below).
- Display a black screen, check that the crc is stable.
- Switch to a white screen, check that the crc is different, but again stable.

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++++++
>  include/drm/drm_dp_helper.h         | 10 ++++++++++
>  4 files changed, 74 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 75a489e..0facff1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1876,6 +1876,29 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>         return 0;
>  }
>
> +static int i915_sink_crc(struct seq_file *m, void *data)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct intel_encoder *encoder;
> +       struct intel_dp *intel_dp = NULL;
> +

We need to grab the modeset lock around this loop.

> +       list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> +               if (encoder->type == INTEL_OUTPUT_EDP) {
> +                       intel_dp = enc_to_intel_dp(&encoder->base);

I think we should skip the output if it's not connected to any pipe or
if the connector is not in DPMS on type. Also, since this is about the
panel I think it's better to loop over connectors.

> +
> +                       intel_dp_sink_crc(intel_dp);
> +                       seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n",
> +                                  intel_dp->sink_crc.r_cr[0],
> +                                  intel_dp->sink_crc.r_cr[1],
> +                                  intel_dp->sink_crc.g_y[0],
> +                                  intel_dp->sink_crc.g_y[1],
> +                                  intel_dp->sink_crc.b_cb[0],
> +                                  intel_dp->sink_crc.b_cb[1]);

Imo it's better to pass an explicit crc array around instead of
storing the last CRC in the intel_dp struct. Also, intel_dp_sink_crc
should return an error code in case the sink doesn't support CRCs or
something else failed.

> +               }
> +       return 0;

We need some return values here for tests I think:
- 0: success, CRC printed with seq_printf.
- -EINVAL: The output isn't enabled at all.
- -ENOTTY: The panel/output doesn't support CRCs:
- Or other error codes for dp aux failed and whatever else can go wrong.

> +}
> +
>  static int i915_energy_uJ(struct seq_file *m, void *data)
>  {
>         struct drm_info_node *node = m->private;
> @@ -3232,6 +3255,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_dpio", i915_dpio_info, 0},
>         {"i915_llc", i915_llc, 0},
>         {"i915_edp_psr_status", i915_edp_psr_status, 0},
> +       {"i915_sink_crc", i915_sink_crc, 0},

We need some room to also support eDP2 and DP1, ... in the future
maybe. Simplest option would be to add an _eDP1 suffix, a control file
like for pipe crcs is imo complete overkill.

>         {"i915_energy_uJ", i915_energy_uJ, 0},
>         {"i915_pc8_status", i915_pc8_status, 0},
>         {"i915_power_domain_info", i915_power_domain_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7df5085..9933327 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2786,6 +2786,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
> +       u8 buf[1];
>
>         if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
>                                            sizeof(intel_dp->dpcd)) == 0)
> @@ -2810,6 +2811,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>                 }
>         }
>
> +       intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf, 1);
> +       intel_dp->sink_crc.supported = buf[0] & DP_TEST_CRC_SUPPORTED;

Personally I'd just recheck this in intel_dp_sink_crc instead of
caching it to avoid stale values and have simpler control flow.

> +
>         if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>               DP_DWN_STRM_PORT_PRESENT))
>                 return true; /* native DP sink */
> @@ -2846,6 +2850,33 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>         ironlake_edp_panel_vdd_off(intel_dp, false);
>  }
>
> +void intel_dp_sink_crc(struct intel_dp *intel_dp)
> +{
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = intel_dig_port->base.base.dev;
> +       struct intel_crtc *intel_crtc =
> +               to_intel_crtc(intel_dig_port->base.base.crtc);
> +
> +       if (!intel_dp->sink_crc.supported)
> +               return;
> +
> +       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, DP_TEST_SINK_START);
> +
> +       /* Wait 2 vblanks to be sure we will have the correct CRC value */
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +       intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR,
> +                                      intel_dp->sink_crc.r_cr, 2);
> +       intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_G_Y,
> +                                      intel_dp->sink_crc.g_y, 2);
> +       intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_B_CB,
> +                                      intel_dp->sink_crc.b_cb, 2);

Iirc you could just read all 6 bytes in one go, dp aux transfers can
be up to 16 bytes.

> +
> +       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
> +                                   ~DP_TEST_SINK_START);

Shouldn't we just write a 0 here?

> +}
> +
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..48533c0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -462,6 +462,13 @@ struct intel_hdmi {
>
>  #define DP_MAX_DOWNSTREAM_PORTS                0x10
>
> +struct sink_crc {
> +       bool supported;
> +       u8 r_cr[2];
> +       u8 g_y[2];
> +       u8 b_cb[2];
> +};
> +
>  struct intel_dp {
>         uint32_t output_reg;
>         uint32_t aux_ch_ctl_reg;
> @@ -487,6 +494,7 @@ struct intel_dp {
>         bool want_panel_vdd;
>         bool psr_setup_done;
>         struct intel_connector *attached_connector;
> +       struct sink_crc sink_crc;
>  };
>
>  struct intel_digital_port {
> @@ -719,6 +727,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>  void intel_dp_check_link_status(struct intel_dp *intel_dp);
> +void intel_dp_sink_crc(struct intel_dp *intel_dp);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>                              struct intel_crtc_config *pipe_config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 1d09050..ba0b90d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h

Better to split this out so non-intel people notice it, just in case.

> @@ -279,11 +279,21 @@
>
>  #define DP_TEST_PATTERN                            0x221
>
> +#define DP_TEST_CRC_R_CR                   0x240
> +#define DP_TEST_CRC_G_Y                            0x242
> +#define DP_TEST_CRC_B_CB                   0x244
> +
> +#define DP_TEST_SINK_MISC                  0x246
> +#define DP_TEST_CRC_SUPPORTED              (1 << 5)
> +
>  #define DP_TEST_RESPONSE                   0x260
>  # define DP_TEST_ACK                       (1 << 0)
>  # define DP_TEST_NAK                       (1 << 1)
>  # define DP_TEST_EDID_CHECKSUM_WRITE       (1 << 2)
>
> +#define DP_TEST_SINK                       0x270
> +#define DP_TEST_SINK_START                 (1 << 0)
> +
>  #define DP_SOURCE_OUI                      0x300
>  #define DP_SINK_OUI                        0x400
>  #define DP_BRANCH_OUI                      0x500
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch