diff mbox

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

Message ID 1389723710-21223-2-git-send-email-rodrigo.vivi@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Jan. 14, 2014, 6:21 p.m. UTC 
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(+)

Comments

Lespiau, Damien Jan. 16, 2014, 5:55 p.m. UTC | #1 
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
Lespiau, Damien Jan. 16, 2014, 6:09 p.m. UTC | #2 
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 :)
diff mbox

Patch

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