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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
