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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux