Re: [PATCH v3 3/3] drm/i915: Use new CRC debugfs API

From: Daniel Vetter
Date: Wed Aug 03 2016 - 04:02:51 EST

Next message: Konstantin Khlebnikov: "Re: Please don't replace numeric parameter like 0444 with macro"
Previous message: Borislav Petkov: "Re: [PATCH 1/1] EDAC, sb_edac: Fix channel reporting on Knights Landing"
Next in thread: Tomeu Vizoso: "Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: Use new CRC debugfs API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jul 22, 2016 at 04:10:45PM +0200, Tomeu Vizoso wrote:
> The core provides now an ABI to userspace for generation of frame CRCs,
> so implement the ->set_crc_source() callback and reuse as much code as
> possible with the previous ABI implementation.
>
> v2:
> - Leave the legacy implementation in place as the ABI implementation
> in the core is incompatible with it.
> v3:
> - Use the "cooked" vblank counter so we have a whole 32 bits.

This should be a separate patch.

> - Make sure we don't mess with the state of the legacy CRC capture
> ABI implementation.

Hm, as for keeping the legacy debugfs interface alive I think it'd be nice
if we can implement the old one internal with the new one. I.e. call the
set_crc_sourc function (after minimal parsing), use the same ringbuffer
and rework the read function to use the same ringbuffer. Or maybe that's
not worth it since the plan is to nuke all that code anyway after a few
months?
-Daniel

>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>

> ---
>
> drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++-------
> drivers/gpu/drm/i915/intel_display.c | 1 +
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 124 ++++++++++++++++++++++++----------
> 4 files changed, 133 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b77d808b71cd..f2726171f7c8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1492,41 +1492,58 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> {
> struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> struct intel_pipe_crc_entry *entry;
> - int head, tail;
> + struct drm_crtc *crtc = intel_get_crtc_for_pipe(&dev_priv->drm, pipe);
> + struct drm_driver *driver = dev_priv->drm.driver;
> + uint32_t crcs[5];
> + int head, tail, ret;
>
> spin_lock(&pipe_crc->lock);
> + if (pipe_crc->source) {
> + if (!pipe_crc->entries) {
> + spin_unlock(&pipe_crc->lock);
> + DRM_DEBUG_KMS("spurious interrupt\n");
> + return;
> + }
>
> - if (!pipe_crc->entries) {
> - spin_unlock(&pipe_crc->lock);
> - DRM_DEBUG_KMS("spurious interrupt\n");
> - return;
> - }
> -
> - head = pipe_crc->head;
> - tail = pipe_crc->tail;
> + head = pipe_crc->head;
> + tail = pipe_crc->tail;
>
> - if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
> - spin_unlock(&pipe_crc->lock);
> - DRM_ERROR("CRC buffer overflowing\n");
> - return;
> - }
> + if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
> + spin_unlock(&pipe_crc->lock);
> + DRM_ERROR("CRC buffer overflowing\n");
> + return;
> + }
>
> - entry = &pipe_crc->entries[head];
> + entry = &pipe_crc->entries[head];
>
> - entry->frame = dev_priv->drm.driver->get_vblank_counter(&dev_priv->drm,
> - pipe);
> - entry->crc[0] = crc0;
> - entry->crc[1] = crc1;
> - entry->crc[2] = crc2;
> - entry->crc[3] = crc3;
> - entry->crc[4] = crc4;
> + entry->frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
> + entry->crc[0] = crc0;
> + entry->crc[1] = crc1;
> + entry->crc[2] = crc2;
> + entry->crc[3] = crc3;
> + entry->crc[4] = crc4;
>
> - head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
> - pipe_crc->head = head;
> + head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
> + pipe_crc->head = head;
>
> - spin_unlock(&pipe_crc->lock);
> + spin_unlock(&pipe_crc->lock);
>
> - wake_up_interruptible(&pipe_crc->wq);
> + wake_up_interruptible(&pipe_crc->wq);
> + } else {
> + spin_unlock(&pipe_crc->lock);
> + spin_lock(&crtc->crc.lock);
> + crcs[0] = crc0;
> + crcs[1] = crc1;
> + crcs[2] = crc2;
> + crcs[3] = crc3;
> + crcs[4] = crc4;
> + ret = drm_crtc_add_crc_entry(crtc, true,
> + drm_accurate_vblank_count(crtc),
> + crcs);
> + spin_unlock(&crtc->crc.lock);
> + if (!ret)
> + wake_up_interruptible(&crtc->crc.wq);
> + }
> }
> #else
> static inline void
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 111b350d1d7e..d91a2f779fb1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14019,6 +14019,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
> .page_flip = intel_crtc_page_flip,
> .atomic_duplicate_state = intel_crtc_duplicate_state,
> .atomic_destroy_state = intel_crtc_destroy_state,
> + .set_crc_source = intel_crtc_set_crc_source,
> };
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5d06cb2425a1..4302cf2cfb04 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1782,6 +1782,8 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
> /* intel_pipe_crc.c */
> int intel_pipe_crc_create(struct drm_minor *minor);
> void intel_pipe_crc_cleanup(struct drm_minor *minor);
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> + size_t *values_cnt);
> extern const struct file_operations i915_display_crc_ctl_fops;
>
> #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 4e6c28ce8ecf..d2f06a1a919a 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -624,15 +624,62 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
> return 0;
> }
>
> +static int do_set_crc_source(struct drm_device *dev, enum pipe pipe,
> + enum intel_pipe_crc_source source)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
> + pipe));
> + u32 val = 0; /* shut up gcc */
> + int ret;
> +
> + if (IS_GEN2(dev))
> + ret = i8xx_pipe_crc_ctl_reg(&source, &val);
> + else if (INTEL_INFO(dev)->gen < 5)
> + ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> + else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> + ret = vlv_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> + else if (IS_GEN5(dev) || IS_GEN6(dev))
> + ret = ilk_pipe_crc_ctl_reg(&source, &val);
> + else
> + ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> +
> + if (ret)
> + return ret;
> +
> + if (source) {
> + /*
> + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> + * enabled and disabled dynamically based on package C states,
> + * user space can't make reliable use of the CRCs, so let's just
> + * completely disable it.
> + */
> + hsw_disable_ips(crtc);
> + }
> +
> + I915_WRITE(PIPE_CRC_CTL(pipe), val);
> + POSTING_READ(PIPE_CRC_CTL(pipe));
> +
> + if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
> + if (IS_G4X(dev))
> + g4x_undo_pipe_scramble_reset(dev, pipe);
> + else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> + vlv_undo_pipe_scramble_reset(dev, pipe);
> + else if (IS_HASWELL(dev) && pipe == PIPE_A)
> + hsw_trans_edp_pipe_A_crc_wa(dev, false);
> +
> + hsw_enable_ips(crtc);
> + }
> +
> + return 0;
> +}
> +
> static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> enum intel_pipe_crc_source source)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> - struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
> - pipe));
> enum intel_display_power_domain power_domain;
> - u32 val = 0; /* shut up gcc */
> int ret;
>
> if (pipe_crc->source == source)
> @@ -648,20 +695,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> return -EIO;
> }
>
> - if (IS_GEN2(dev))
> - ret = i8xx_pipe_crc_ctl_reg(&source, &val);
> - else if (INTEL_INFO(dev)->gen < 5)
> - ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> - else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> - ret = vlv_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> - else if (IS_GEN5(dev) || IS_GEN6(dev))
> - ret = ilk_pipe_crc_ctl_reg(&source, &val);
> - else
> - ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> -
> - if (ret != 0)
> - goto out;
> -
> /* none -> real source transition */
> if (source) {
> struct intel_pipe_crc_entry *entries;
> @@ -677,14 +710,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> goto out;
> }
>
> - /*
> - * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> - * enabled and disabled dynamically based on package C states,
> - * user space can't make reliable use of the CRCs, so let's just
> - * completely disable it.
> - */
> - hsw_disable_ips(crtc);
> -
> spin_lock_irq(&pipe_crc->lock);
> kfree(pipe_crc->entries);
> pipe_crc->entries = entries;
> @@ -693,10 +718,11 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> spin_unlock_irq(&pipe_crc->lock);
> }
>
> - pipe_crc->source = source;
> + ret = do_set_crc_source(dev, pipe, source);
> + if (ret)
> + goto out;
>
> - I915_WRITE(PIPE_CRC_CTL(pipe), val);
> - POSTING_READ(PIPE_CRC_CTL(pipe));
> + pipe_crc->source = source;
>
> /* real source -> none transition */
> if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
> @@ -720,15 +746,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> spin_unlock_irq(&pipe_crc->lock);
>
> kfree(entries);
> -
> - if (IS_G4X(dev))
> - g4x_undo_pipe_scramble_reset(dev, pipe);
> - else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> - vlv_undo_pipe_scramble_reset(dev, pipe);
> - else if (IS_HASWELL(dev) && pipe == PIPE_A)
> - hsw_trans_edp_pipe_A_crc_wa(dev, false);
> -
> - hsw_enable_ips(crtc);
> }
>
> ret = 0;
> @@ -821,6 +838,11 @@ display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
> {
> int i;
>
> + if (!buf) {
> + *s = INTEL_PIPE_CRC_SOURCE_NONE;
> + return 0;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
> if (!strcmp(buf, pipe_crc_sources[i])) {
> *s = i;
> @@ -949,3 +971,31 @@ void intel_pipe_crc_cleanup(struct drm_minor *minor)
> drm_debugfs_remove_files(info_list, 1, minor);
> }
> }
> +
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> + size_t *values_cnt)
> +{
> + struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> + enum intel_display_power_domain power_domain;
> + enum intel_pipe_crc_source source;
> + int ret;
> +
> + if (display_crc_ctl_parse_source(source_name, &source) < 0) {
> + DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> + return -EINVAL;
> + }
> +
> + power_domain = POWER_DOMAIN_PIPE(crtc->index);
> + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
> + DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
> + return -EIO;
> + }
> +
> + ret = do_set_crc_source(crtc->dev, crtc->index, source);
> +
> + intel_display_power_put(dev_priv, power_domain);
> +
> + *values_cnt = 5;
> +
> + return ret;
> +}
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Next message: Konstantin Khlebnikov: "Re: Please don't replace numeric parameter like 0444 with macro"
Previous message: Borislav Petkov: "Re: [PATCH 1/1] EDAC, sb_edac: Fix channel reporting on Knights Landing"
Next in thread: Tomeu Vizoso: "Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: Use new CRC debugfs API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]