[RESEND PATCH v14 0/2] New debugfs API for capturing CRC of frames

[RESEND PATCH v14 0/2] New debugfs API for capturing CRC of frames

[Tomeu Vizoso]

Hi,

here are the last two patches that remain to be merged in this series,
rebased on today's drm-tip.

Thanks,

Tomeu


Tomeu Vizoso (2):
  drm/i915: Use new CRC debugfs API
  drm/i915: Put "cooked" vlank counters in frame CRC lines

 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_irq.c       | 77 ++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h      |  6 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++++++++++++++++++++++++++++++-----
 5 files changed, 142 insertions(+), 37 deletions(-)

-- 
2.9.3

[RESEND PATCH v14 1/2] drm/i915: Use new CRC debugfs API

[Tomeu Vizoso]

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.

When handling the pageflip interrupt, we skip 1 or 2 frames depending on
the HW because they contain wrong values. For the legacy ABI for
generating frame CRCs, this was done in userspace but now that we have a
generic ABI it's better if it's not exposed by the kernel.

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.
    - Make sure we don't mess with the state of the legacy CRC capture
      ABI implementation.
v4:
    - Keep use of get_vblank_counter as in the legacy code, will be
      changed in a followup commit.

v5:
    - Skip first frame or two as it's known that they contain wrong
      data.
    - A few fixes suggested by Emil Velikov.

v6:
    - Rework programming of the HW registers to preserve previous
      behavior.

v7:
    - Address whitespace issue.
    - Added a comment on why in the implementation of the new ABI we
      skip the 1st or 2nd frames.

v9:
    - Add stub for intel_crtc_set_crc_source.

v12:
    - Rebased.
    - Remove stub for intel_crtc_set_crc_source and instead set the
      callback to NULL (Jani Nikula).

v15:
    - Rebased.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>

irq

---

 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_irq.c       | 77 ++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h      |  6 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++++++++++++++++++++++++++++++-----
 5 files changed, 142 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ffeebf869e36..7a0eab675031 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1810,6 +1810,7 @@ struct intel_pipe_crc {
 	enum intel_pipe_crc_source source;
 	int head, tail;
 	wait_queue_head_t wq;
+	int skipped;
 };
 
 struct i915_frontbuffer_tracking {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a0e70f5b3aad..b9beb5955dae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1553,41 +1553,68 @@ 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;
+	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+	struct drm_driver *driver = dev_priv->drm.driver;
+	uint32_t crcs[5];
 	int head, tail;
+	u32 frame;
 
 	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 {
+		/*
+		 * For some not yet identified reason, the first CRC is
+		 * bonkers. So let's just wait for the next vblank and read
+		 * out the buggy result.
+		 *
+		 * On CHV sometimes the second CRC is bonkers as well, so
+		 * don't trust that one either.
+		 */
+		if (pipe_crc->skipped == 0 ||
+		    (IS_CHERRYVIEW(dev_priv) && pipe_crc->skipped == 1)) {
+			pipe_crc->skipped++;
+			spin_unlock(&pipe_crc->lock);
+			return;
+		}
+		spin_unlock(&pipe_crc->lock);
+		crcs[0] = crc0;
+		crcs[1] = crc1;
+		crcs[2] = crc2;
+		crcs[3] = crc3;
+		crcs[4] = crc4;
+		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+		drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
+	}
 }
 #else
 static inline void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2150a64860c..56047018391c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14737,6 +14737,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 6b02dac6ea26..84258df3e8f1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1880,5 +1880,11 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
 void intel_pipe_crc_cleanup(struct drm_minor *minor);
+#ifdef CONFIG_DEBUG_FS
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
+			      size_t *values_cnt);
+#else
+#define intel_crtc_set_crc_source NULL
+#endif
 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 ef0c0e195164..0f1da810cff0 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -613,6 +613,22 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
+static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
+			       enum pipe pipe,
+			       enum intel_pipe_crc_source *source, u32 *val)
+{
+	if (IS_GEN2(dev_priv))
+		return i8xx_pipe_crc_ctl_reg(source, val);
+	else if (INTEL_GEN(dev_priv) < 5)
+		return i9xx_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
+		return ilk_pipe_crc_ctl_reg(source, val);
+	else
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+}
+
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
 			       enum intel_pipe_crc_source source)
@@ -636,17 +652,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	if (IS_GEN2(dev_priv))
-		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
-	else if (INTEL_GEN(dev_priv) < 5)
-		ret = i9xx_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		ret = vlv_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
-	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
-		ret = ilk_pipe_crc_ctl_reg(&source, &val);
-	else
-		ret = ivb_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
-
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
 	if (ret != 0)
 		goto out;
 
@@ -687,7 +693,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 	POSTING_READ(PIPE_CRC_CTL(pipe));
 
 	/* real source -> none transition */
-	if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+	if (!source) {
 		struct intel_pipe_crc_entry *entries;
 		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
 								  pipe);
@@ -809,6 +815,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;
@@ -937,3 +948,62 @@ 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;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum intel_display_power_domain power_domain;
+	enum intel_pipe_crc_source source;
+	u32 val = 0; /* shut up gcc */
+	int ret = 0;
+
+	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 = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	if (ret != 0)
+		goto out;
+
+	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(intel_crtc);
+	}
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+
+	if (!source) {
+		if (IS_G4X(dev_priv))
+			g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
+		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
+		else if (IS_HASWELL(dev_priv) && crtc->index == PIPE_A)
+			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
+
+		hsw_enable_ips(intel_crtc);
+	}
+
+	pipe_crc->skipped = 0;
+	*values_cnt = 5;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
+}
-- 
2.9.3

[RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

[Tomeu Vizoso]

Use drm_accurate_vblank_count so we have the full 32 bit to represent
the frame counter and userspace has a simpler way of knowing when the
counter wraps around.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---

 drivers/gpu/drm/i915/i915_irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b9beb5955dae..75fb1f66cc0c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 	struct drm_driver *driver = dev_priv->drm.driver;
 	uint32_t crcs[5];
 	int head, tail;
-	u32 frame;
 
 	spin_lock(&pipe_crc->lock);
 	if (pipe_crc->source) {
@@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 		crcs[2] = crc2;
 		crcs[3] = crc3;
 		crcs[4] = crc4;
-		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
-		drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
+		drm_crtc_add_crc_entry(&crtc->base, true,
+				       drm_accurate_vblank_count(&crtc->base),
+				       crcs);
 	}
 }
 #else
-- 
2.9.3

Re: [RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 02:43:05PM +0100, Tomeu Vizoso wrote:
> Use drm_accurate_vblank_count so we have the full 32 bit to represent
> the frame counter and userspace has a simpler way of knowing when the
> counter wraps around.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> Reviewed-by: Robert Foss <robert.foss@collabora.com>
> ---
> 
>  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b9beb5955dae..75fb1f66cc0c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  	struct drm_driver *driver = dev_priv->drm.driver;
>  	uint32_t crcs[5];
>  	int head, tail;
> -	u32 frame;
>  
>  	spin_lock(&pipe_crc->lock);
>  	if (pipe_crc->source) {
> @@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  		crcs[2] = crc2;
>  		crcs[3] = crc3;
>  		crcs[4] = crc4;
> -		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
> -		drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
> +		drm_crtc_add_crc_entry(&crtc->base, true,
> +				       drm_accurate_vblank_count(&crtc->base),

My assumption would be that this gets called after the vblank irq
handler, so using the _accurate version seems a bit overkill.

> +				       crcs);
>  	}
>  }
>  #else
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

[Daniel Vetter]

On Tue, Jan 10, 2017 at 05:54:57PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 10, 2017 at 02:43:05PM +0100, Tomeu Vizoso wrote:
> > Use drm_accurate_vblank_count so we have the full 32 bit to represent
> > the frame counter and userspace has a simpler way of knowing when the
> > counter wraps around.
> > 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> > Reviewed-by: Robert Foss <robert.foss@collabora.com>
> > ---
> > 
> >  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index b9beb5955dae..75fb1f66cc0c 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >  	struct drm_driver *driver = dev_priv->drm.driver;
> >  	uint32_t crcs[5];
> >  	int head, tail;
> > -	u32 frame;
> >  
> >  	spin_lock(&pipe_crc->lock);
> >  	if (pipe_crc->source) {
> > @@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >  		crcs[2] = crc2;
> >  		crcs[3] = crc3;
> >  		crcs[4] = crc4;
> > -		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
> > -		drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
> > +		drm_crtc_add_crc_entry(&crtc->base, true,
> > +				       drm_accurate_vblank_count(&crtc->base),
> 
> My assumption would be that this gets called after the vblank irq
> handler, so using the _accurate version seems a bit overkill.

Since we're at like v15 of this I figured I'll pull this in, and we can
polish this a bit more later. Tomeu, can you pls do that follow-up patch
and get Ville to review+merge it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

[Tomeu Vizoso]

On 10 January 2017 at 17:31, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jan 10, 2017 at 05:54:57PM +0200, Ville Syrjälä wrote:
>> On Tue, Jan 10, 2017 at 02:43:05PM +0100, Tomeu Vizoso wrote:
>> > Use drm_accurate_vblank_count so we have the full 32 bit to represent
>> > the frame counter and userspace has a simpler way of knowing when the
>> > counter wraps around.
>> >
>> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>> > Reviewed-by: Robert Foss <robert.foss@collabora.com>
>> > ---
>> >
>> >  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index b9beb5955dae..75fb1f66cc0c 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>> >     struct drm_driver *driver = dev_priv->drm.driver;
>> >     uint32_t crcs[5];
>> >     int head, tail;
>> > -   u32 frame;
>> >
>> >     spin_lock(&pipe_crc->lock);
>> >     if (pipe_crc->source) {
>> > @@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>> >             crcs[2] = crc2;
>> >             crcs[3] = crc3;
>> >             crcs[4] = crc4;
>> > -           frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
>> > -           drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
>> > +           drm_crtc_add_crc_entry(&crtc->base, true,
>> > +                                  drm_accurate_vblank_count(&crtc->base),
>>
>> My assumption would be that this gets called after the vblank irq
>> handler, so using the _accurate version seems a bit overkill.
>
> Since we're at like v15 of this I figured I'll pull this in, and we can
> polish this a bit more later. Tomeu, can you pls do that follow-up patch
> and get Ville to review+merge it.

At least on the SKL and SNB I have here, the -sequence subtests in
kms_pipe_crc_basic fail if I replace the call to
drm_accurate_vblank_count with drm_crtc_vblank_count.

Any ideas on why this could be?

Thanks,

Tomeu

Re: [Intel-gfx] [RESEND PATCH v14 2/2] drm/i915: Put "cooked" vlank counters in frame CRC lines

[Daniel Vetter]

On Mon, Jan 16, 2017 at 10:12:36AM +0100, Tomeu Vizoso wrote:
> On 10 January 2017 at 17:31, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Jan 10, 2017 at 05:54:57PM +0200, Ville Syrjälä wrote:
> >> On Tue, Jan 10, 2017 at 02:43:05PM +0100, Tomeu Vizoso wrote:
> >> > Use drm_accurate_vblank_count so we have the full 32 bit to represent
> >> > the frame counter and userspace has a simpler way of knowing when the
> >> > counter wraps around.
> >> >
> >> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> >> > Reviewed-by: Robert Foss <robert.foss@collabora.com>
> >> > ---
> >> >
> >> >  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> > index b9beb5955dae..75fb1f66cc0c 100644
> >> > --- a/drivers/gpu/drm/i915/i915_irq.c
> >> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> > @@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >> >     struct drm_driver *driver = dev_priv->drm.driver;
> >> >     uint32_t crcs[5];
> >> >     int head, tail;
> >> > -   u32 frame;
> >> >
> >> >     spin_lock(&pipe_crc->lock);
> >> >     if (pipe_crc->source) {
> >> > @@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >> >             crcs[2] = crc2;
> >> >             crcs[3] = crc3;
> >> >             crcs[4] = crc4;
> >> > -           frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
> >> > -           drm_crtc_add_crc_entry(&crtc->base, true, frame, crcs);
> >> > +           drm_crtc_add_crc_entry(&crtc->base, true,
> >> > +                                  drm_accurate_vblank_count(&crtc->base),
> >>
> >> My assumption would be that this gets called after the vblank irq
> >> handler, so using the _accurate version seems a bit overkill.
> >
> > Since we're at like v15 of this I figured I'll pull this in, and we can
> > polish this a bit more later. Tomeu, can you pls do that follow-up patch
> > and get Ville to review+merge it.
> 
> At least on the SKL and SNB I have here, the -sequence subtests in
> kms_pipe_crc_basic fail if I replace the call to
> drm_accurate_vblank_count with drm_crtc_vblank_count.
> 
> Any ideas on why this could be?

No idea at all, on a quick check things seem ordered correctly. Can you
pls paste how the test falls over?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch