On Thu, 23 Jan 2014, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
