Pipe CRCs v1

Pipe CRCs v1

[Damien Lespiau]

This series exposes the pipe CRCs on ivybridge through debugfs. It's based on
the initial work from Shuang He, with some improvements to have a nice user
space API.

There are several points in the display pipeline where CRCs can be computed
on the bits flowing there. For instance, it's usually possible to compute
the CRCs of the primary plane, the sprite plane or the CRCs of the bits
after the panel fitter (collectively called pipe CRCs).

An intel-gpu-tools series will follow with helpers to use the feature from
tests and basic testing.

Further work items:
  * make it work on other platforms
  * expose other CRCs than just the pipe CRCs (transcoders, ddi, ...)
  * implement poll() for the result files

-- 
Damien

 drivers/gpu/drm/i915/i915_debugfs.c | 503 ++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_dma.c     |   2 +
 drivers/gpu/drm/i915/i915_drv.h     |  31 ++
 drivers/gpu/drm/i915/i915_irq.c     |  49 +++
 drivers/gpu/drm/i915/i915_reg.h     |  36 +-
 5 files changed, 594 insertions(+), 27 deletions(-)

[PATCH 01/16] drm/i915: Expose latest 200 CRC value for pipe through debugfs

[Damien Lespiau]

From: Shuang He <shuang.he@intel.com>

There are several points in the display pipeline where CRCs can be
computed on the bits flowing there. For instance, it's usually possible
to compute the CRCs of the primary plane, the sprite plane or the CRCs
of the bits after the panel fitter (collectively called pipe CRCs).

v2: Quite a bit of rework here and there (Damien)

Signed-off-by: Shuang He <shuang.he@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     | 16 ++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c     | 35 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     | 36 +++++++++++++++++++++++++++++++++++-
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72d0458..e1d45aa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1732,6 +1732,36 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_pipe_crc(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = (enum pipe)node->info_ent->data;
+	const struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
+	int i;
+	int start;
+
+	if (!IS_IVYBRIDGE(dev)) {
+		seq_puts(m, "unsupported\n");
+		return 0;
+	}
+
+	start = atomic_read(&pipe_crc->slot) + 1;
+	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
+	for (i = 0; i < INTEL_PIPE_CRC_ENTRIES_NR; i++) {
+		const struct intel_pipe_crc_entry *entry =
+			&pipe_crc->entries[(start + i) %
+					   INTEL_PIPE_CRC_ENTRIES_NR];
+
+		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
+			   entry->crc[0], entry->crc[1], entry->crc[2],
+			   entry->crc[3], entry->crc[4]);
+	}
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2247,6 +2277,9 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
+	{"i915_pipe_A_crc", i915_pipe_crc, 0, (void *)PIPE_A},
+	{"i915_pipe_B_crc", i915_pipe_crc, 0, (void *)PIPE_B},
+	{"i915_pipe_C_crc", i915_pipe_crc, 0, (void *)PIPE_C},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6106d3d..6855d91 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1219,6 +1219,18 @@ struct i915_package_c8 {
 	} regsave;
 };
 
+struct intel_pipe_crc_entry {
+	uint32_t timestamp;
+	uint32_t crc[5];
+};
+
+#define INTEL_PIPE_CRC_ENTRIES_NR	200
+struct intel_pipe_crc {
+	struct intel_pipe_crc_entry entries[INTEL_PIPE_CRC_ENTRIES_NR];
+	enum intel_pipe_crc_source source;
+	atomic_t slot;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1423,6 +1435,10 @@ typedef struct drm_i915_private {
 	struct i915_dri1_state dri1;
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
+
+#ifdef CONFIG_DEBUG_FS
+	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
+#endif
 } drm_i915_private_t;
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 26753b6..d2074f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1188,6 +1188,32 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
+#if defined(CONFIG_DEBUG_FS)
+static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
+	struct intel_pipe_crc_entry *entry;
+	ktime_t now;
+	int ts, slot;
+
+	now = ktime_get();
+	ts = ktime_to_us(now);
+
+	slot = (atomic_read(&pipe_crc->slot) + 1) % INTEL_PIPE_CRC_ENTRIES_NR;
+	entry = &pipe_crc->entries[slot];
+	entry->timestamp = ts;
+	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
+	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
+	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
+	entry->crc[3] = I915_READ(PIPE_CRC_RES_4_IVB(pipe));
+	entry->crc[4] = I915_READ(PIPE_CRC_RES_5_IVB(pipe));
+	atomic_set(&dev_priv->pipe_crc[pipe].slot, slot);
+}
+#else
+static void ivb_pipe_crc_update(struct drm_device *dev, int pipe) {}
+#endif
+
 /* The RPS events need forcewake, so we add them to a work queue and mask their
  * IMR bits until the work is done. Other interrupts can be processed without
  * the work queue. */
@@ -1366,6 +1392,15 @@ static void ivb_err_int_handler(struct drm_device *dev)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_C, false))
 			DRM_DEBUG_DRIVER("Pipe C FIFO underrun\n");
 
+	if (err_int & ERR_INT_PIPE_CRC_DONE_A)
+		ivb_pipe_crc_update(dev, PIPE_A);
+
+	if (err_int & ERR_INT_PIPE_CRC_DONE_B)
+		ivb_pipe_crc_update(dev, PIPE_B);
+
+	if (err_int & ERR_INT_PIPE_CRC_DONE_C)
+		ivb_pipe_crc_update(dev, PIPE_C);
+
 	I915_WRITE(GEN7_ERR_INT, err_int);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 13153c3..4d01eaf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -722,8 +722,11 @@
 #define GEN7_ERR_INT	0x44040
 #define   ERR_INT_POISON		(1<<31)
 #define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
+#define   ERR_INT_PIPE_CRC_DONE_C	(1<<8)
 #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
+#define   ERR_INT_PIPE_CRC_DONE_B	(1<<5)
 #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
+#define   ERR_INT_PIPE_CRC_DONE_A	(1<<2)
 #define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
 #define   ERR_INT_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
 
@@ -1835,6 +1838,38 @@
  * Display engine regs
  */
 
+/* Pipe A CRC regs */
+#define _PIPE_CRC_CTL_A		(dev_priv->info->display_mmio_offset + 0x60050)
+#define   PIPE_CRC_ENABLE		(1 << 31)
+#define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
+#define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
+#define   PIPE_CRC_SOURCE_PF_IVB	(2 << 29)
+#define _PIPE_CRC_RES_1_A_IVB	(dev_priv->info->display_mmio_offset + 0x60064)
+#define _PIPE_CRC_RES_2_A_IVB	(dev_priv->info->display_mmio_offset + 0x60068)
+#define _PIPE_CRC_RES_3_A_IVB	(dev_priv->info->display_mmio_offset + 0x6006c)
+#define _PIPE_CRC_RES_4_A_IVB	(dev_priv->info->display_mmio_offset + 0x60070)
+#define _PIPE_CRC_RES_5_A_IVB	(dev_priv->info->display_mmio_offset + 0x60074)
+
+/* Pipe B CRC regs */
+#define _PIPE_CRC_CTL_B		(dev_priv->info->display_mmio_offset + 0x61050)
+#define _PIPE_CRC_RES_1_B_IVB	(dev_priv->info->display_mmio_offset + 0x61064)
+#define _PIPE_CRC_RES_2_B_IVB	(dev_priv->info->display_mmio_offset + 0x61068)
+#define _PIPE_CRC_RES_3_B_IVB	(dev_priv->info->display_mmio_offset + 0x6106c)
+#define _PIPE_CRC_RES_4_B_IVB	(dev_priv->info->display_mmio_offset + 0x61070)
+#define _PIPE_CRC_RES_5_B_IVB	(dev_priv->info->display_mmio_offset + 0x61074)
+
+#define PIPE_CRC_CTL(pipe)	_PIPE(pipe, _PIPE_CRC_CTL_A, _PIPE_CRC_CTL_B)
+#define PIPE_CRC_RES_1_IVB(pipe)	\
+	_PIPE(pipe, _PIPE_CRC_RES_1_A_IVB, _PIPE_CRC_RES_1_B_IVB)
+#define PIPE_CRC_RES_2_IVB(pipe)	\
+	_PIPE(pipe, _PIPE_CRC_RES_2_A_IVB, _PIPE_CRC_RES_2_B_IVB)
+#define PIPE_CRC_RES_3_IVB(pipe)	\
+	_PIPE(pipe, _PIPE_CRC_RES_3_A_IVB, _PIPE_CRC_RES_3_B_IVB)
+#define PIPE_CRC_RES_4_IVB(pipe)	\
+	_PIPE(pipe, _PIPE_CRC_RES_4_A_IVB, _PIPE_CRC_RES_4_B_IVB)
+#define PIPE_CRC_RES_5_IVB(pipe)	\
+	_PIPE(pipe, _PIPE_CRC_RES_5_A_IVB, _PIPE_CRC_RES_5_B_IVB)
+
 /* Pipe A timing regs */
 #define _HTOTAL_A	(dev_priv->info->display_mmio_offset + 0x60000)
 #define _HBLANK_A	(dev_priv->info->display_mmio_offset + 0x60004)
@@ -1857,7 +1892,6 @@
 #define _BCLRPAT_B	(dev_priv->info->display_mmio_offset + 0x61020)
 #define _VSYNCSHIFT_B	(dev_priv->info->display_mmio_offset + 0x61028)
 
-
 #define HTOTAL(trans) _TRANSCODER(trans, _HTOTAL_A, _HTOTAL_B)
 #define HBLANK(trans) _TRANSCODER(trans, _HBLANK_A, _HBLANK_B)
 #define HSYNC(trans) _TRANSCODER(trans, _HSYNC_A, _HSYNC_B)
-- 
1.8.3.1

Re: [PATCH 01/16] drm/i915: Expose latest 200 CRC value for pipe through debugfs

[Ville Syrjälä]

On Tue, Oct 15, 2013 at 06:55:27PM +0100, Damien Lespiau wrote:
> From: Shuang He <shuang.he@intel.com>
> 
> There are several points in the display pipeline where CRCs can be
> computed on the bits flowing there. For instance, it's usually possible
> to compute the CRCs of the primary plane, the sprite plane or the CRCs
> of the bits after the panel fitter (collectively called pipe CRCs).
> 
> v2: Quite a bit of rework here and there (Damien)
> 
> Signed-off-by: Shuang He <shuang.he@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c     | 35 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     | 36 +++++++++++++++++++++++++++++++++++-
>  4 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72d0458..e1d45aa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1732,6 +1732,36 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static int i915_pipe_crc(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *) m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe pipe = (enum pipe)node->info_ent->data;
> +	const struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> +	int i;
> +	int start;
> +
> +	if (!IS_IVYBRIDGE(dev)) {
> +		seq_puts(m, "unsupported\n");
> +		return 0;
> +	}
> +
> +	start = atomic_read(&pipe_crc->slot) + 1;
> +	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> +	for (i = 0; i < INTEL_PIPE_CRC_ENTRIES_NR; i++) {
> +		const struct intel_pipe_crc_entry *entry =
> +			&pipe_crc->entries[(start + i) %
> +					   INTEL_PIPE_CRC_ENTRIES_NR];
> +
> +		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> +			   entry->crc[0], entry->crc[1], entry->crc[2],
> +			   entry->crc[3], entry->crc[4]);
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  i915_wedged_get(void *data, u64 *val)
>  {
> @@ -2247,6 +2277,9 @@ static struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
>  	{"i915_energy_uJ", i915_energy_uJ, 0},
>  	{"i915_pc8_status", i915_pc8_status, 0},
> +	{"i915_pipe_A_crc", i915_pipe_crc, 0, (void *)PIPE_A},
> +	{"i915_pipe_B_crc", i915_pipe_crc, 0, (void *)PIPE_B},
> +	{"i915_pipe_C_crc", i915_pipe_crc, 0, (void *)PIPE_C},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6106d3d..6855d91 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1219,6 +1219,18 @@ struct i915_package_c8 {
>  	} regsave;
>  };
>  
> +struct intel_pipe_crc_entry {
> +	uint32_t timestamp;
> +	uint32_t crc[5];
> +};
> +
> +#define INTEL_PIPE_CRC_ENTRIES_NR	200
> +struct intel_pipe_crc {
> +	struct intel_pipe_crc_entry entries[INTEL_PIPE_CRC_ENTRIES_NR];
> +	enum intel_pipe_crc_source source;
> +	atomic_t slot;
> +};
> +
>  typedef struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1423,6 +1435,10 @@ typedef struct drm_i915_private {
>  	struct i915_dri1_state dri1;
>  	/* Old ums support infrastructure, same warning applies. */
>  	struct i915_ums_state ums;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
> +#endif

Should we keep this in intel_crtc perhaps?

>  } drm_i915_private_t;
>  
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 26753b6..d2074f1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1188,6 +1188,32 @@ static void dp_aux_irq_handler(struct drm_device *dev)
>  	wake_up_all(&dev_priv->gmbus_wait_queue);
>  }
>  
> +#if defined(CONFIG_DEBUG_FS)
> +static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> +	struct intel_pipe_crc_entry *entry;
> +	ktime_t now;
> +	int ts, slot;
> +
> +	now = ktime_get();
> +	ts = ktime_to_us(now);
> +
> +	slot = (atomic_read(&pipe_crc->slot) + 1) % INTEL_PIPE_CRC_ENTRIES_NR;

Looks like using atomic_t for 'slot' doesn't really help us in any way.

> +	entry = &pipe_crc->entries[slot];
> +	entry->timestamp = ts;
> +	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
> +	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
> +	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
> +	entry->crc[3] = I915_READ(PIPE_CRC_RES_4_IVB(pipe));
> +	entry->crc[4] = I915_READ(PIPE_CRC_RES_5_IVB(pipe));
> +	atomic_set(&dev_priv->pipe_crc[pipe].slot, slot);
> +}
> +#else
> +static void ivb_pipe_crc_update(struct drm_device *dev, int pipe) {}
> +#endif
> +
>  /* The RPS events need forcewake, so we add them to a work queue and mask their
>   * IMR bits until the work is done. Other interrupts can be processed without
>   * the work queue. */
> @@ -1366,6 +1392,15 @@ static void ivb_err_int_handler(struct drm_device *dev)
>  		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_C, false))
>  			DRM_DEBUG_DRIVER("Pipe C FIFO underrun\n");
>  
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_A)
> +		ivb_pipe_crc_update(dev, PIPE_A);
> +
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_B)
> +		ivb_pipe_crc_update(dev, PIPE_B);
> +
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_C)
> +		ivb_pipe_crc_update(dev, PIPE_C);
> +
>  	I915_WRITE(GEN7_ERR_INT, err_int);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 13153c3..4d01eaf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -722,8 +722,11 @@
>  #define GEN7_ERR_INT	0x44040
>  #define   ERR_INT_POISON		(1<<31)
>  #define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
> +#define   ERR_INT_PIPE_CRC_DONE_C	(1<<8)
>  #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
> +#define   ERR_INT_PIPE_CRC_DONE_B	(1<<5)
>  #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
> +#define   ERR_INT_PIPE_CRC_DONE_A	(1<<2)
>  #define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
>  #define   ERR_INT_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
>  
> @@ -1835,6 +1838,38 @@
>   * Display engine regs
>   */
>  
> +/* Pipe A CRC regs */
> +#define _PIPE_CRC_CTL_A		(dev_priv->info->display_mmio_offset + 0x60050)
> +#define   PIPE_CRC_ENABLE		(1 << 31)
> +#define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
> +#define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> +#define   PIPE_CRC_SOURCE_PF_IVB	(2 << 29)
> +#define _PIPE_CRC_RES_1_A_IVB	(dev_priv->info->display_mmio_offset + 0x60064)
> +#define _PIPE_CRC_RES_2_A_IVB	(dev_priv->info->display_mmio_offset + 0x60068)
> +#define _PIPE_CRC_RES_3_A_IVB	(dev_priv->info->display_mmio_offset + 0x6006c)
> +#define _PIPE_CRC_RES_4_A_IVB	(dev_priv->info->display_mmio_offset + 0x60070)
> +#define _PIPE_CRC_RES_5_A_IVB	(dev_priv->info->display_mmio_offset + 0x60074)
> +
> +/* Pipe B CRC regs */
> +#define _PIPE_CRC_CTL_B		(dev_priv->info->display_mmio_offset + 0x61050)
> +#define _PIPE_CRC_RES_1_B_IVB	(dev_priv->info->display_mmio_offset + 0x61064)
> +#define _PIPE_CRC_RES_2_B_IVB	(dev_priv->info->display_mmio_offset + 0x61068)
> +#define _PIPE_CRC_RES_3_B_IVB	(dev_priv->info->display_mmio_offset + 0x6106c)
> +#define _PIPE_CRC_RES_4_B_IVB	(dev_priv->info->display_mmio_offset + 0x61070)
> +#define _PIPE_CRC_RES_5_B_IVB	(dev_priv->info->display_mmio_offset + 0x61074)
> +
> +#define PIPE_CRC_CTL(pipe)	_PIPE(pipe, _PIPE_CRC_CTL_A, _PIPE_CRC_CTL_B)
> +#define PIPE_CRC_RES_1_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_1_A_IVB, _PIPE_CRC_RES_1_B_IVB)
> +#define PIPE_CRC_RES_2_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_2_A_IVB, _PIPE_CRC_RES_2_B_IVB)
> +#define PIPE_CRC_RES_3_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_3_A_IVB, _PIPE_CRC_RES_3_B_IVB)
> +#define PIPE_CRC_RES_4_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_4_A_IVB, _PIPE_CRC_RES_4_B_IVB)
> +#define PIPE_CRC_RES_5_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_5_A_IVB, _PIPE_CRC_RES_5_B_IVB)
> +
>  /* Pipe A timing regs */
>  #define _HTOTAL_A	(dev_priv->info->display_mmio_offset + 0x60000)
>  #define _HBLANK_A	(dev_priv->info->display_mmio_offset + 0x60004)
> @@ -1857,7 +1892,6 @@
>  #define _BCLRPAT_B	(dev_priv->info->display_mmio_offset + 0x61020)
>  #define _VSYNCSHIFT_B	(dev_priv->info->display_mmio_offset + 0x61028)
>  
> -
>  #define HTOTAL(trans) _TRANSCODER(trans, _HTOTAL_A, _HTOTAL_B)
>  #define HBLANK(trans) _TRANSCODER(trans, _HBLANK_A, _HBLANK_B)
>  #define HSYNC(trans) _TRANSCODER(trans, _HSYNC_A, _HSYNC_B)
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 01/16] drm/i915: Expose latest 200 CRC value for pipe through debugfs

[He, Shuang]

Thank you Danmien for helping with this.
It's moving so fast. I'm looking forward we could create some automation test for media also

Thanks
	--Shuang

> -----Original Message-----
> From: Lespiau, Damien
> Sent: Wednesday, October 16, 2013 1:55 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: He, Shuang
> Subject: [PATCH 01/16] drm/i915: Expose latest 200 CRC value for pipe through
> debugfs
> 
> From: Shuang He <shuang.he@intel.com>
> 
> There are several points in the display pipeline where CRCs can be computed
> on the bits flowing there. For instance, it's usually possible to compute the
> CRCs of the primary plane, the sprite plane or the CRCs of the bits after the
> panel fitter (collectively called pipe CRCs).
> 
> v2: Quite a bit of rework here and there (Damien)
> 
> Signed-off-by: Shuang He <shuang.he@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 33
> +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c     | 35
> +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     | 36
> +++++++++++++++++++++++++++++++++++-
>  4 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72d0458..e1d45aa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1732,6 +1732,36 @@ static int i915_pc8_status(struct seq_file *m, void
> *unused)
>  	return 0;
>  }
> 
> +static int i915_pipe_crc(struct seq_file *m, void *data) {
> +	struct drm_info_node *node = (struct drm_info_node *) m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe pipe = (enum pipe)node->info_ent->data;
> +	const struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> +	int i;
> +	int start;
> +
> +	if (!IS_IVYBRIDGE(dev)) {
> +		seq_puts(m, "unsupported\n");
> +		return 0;
> +	}
> +
> +	start = atomic_read(&pipe_crc->slot) + 1;
> +	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4
> CRC5\n");
> +	for (i = 0; i < INTEL_PIPE_CRC_ENTRIES_NR; i++) {
> +		const struct intel_pipe_crc_entry *entry =
> +			&pipe_crc->entries[(start + i) %
> +					   INTEL_PIPE_CRC_ENTRIES_NR];
> +
> +		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> +			   entry->crc[0], entry->crc[1], entry->crc[2],
> +			   entry->crc[3], entry->crc[4]);
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  i915_wedged_get(void *data, u64 *val)
>  {
> @@ -2247,6 +2277,9 @@ static struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
>  	{"i915_energy_uJ", i915_energy_uJ, 0},
>  	{"i915_pc8_status", i915_pc8_status, 0},
> +	{"i915_pipe_A_crc", i915_pipe_crc, 0, (void *)PIPE_A},
> +	{"i915_pipe_B_crc", i915_pipe_crc, 0, (void *)PIPE_B},
> +	{"i915_pipe_C_crc", i915_pipe_crc, 0, (void *)PIPE_C},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6106d3d..6855d91 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1219,6 +1219,18 @@ struct i915_package_c8 {
>  	} regsave;
>  };
> 
> +struct intel_pipe_crc_entry {
> +	uint32_t timestamp;
> +	uint32_t crc[5];
> +};
> +
> +#define INTEL_PIPE_CRC_ENTRIES_NR	200
> +struct intel_pipe_crc {
> +	struct intel_pipe_crc_entry entries[INTEL_PIPE_CRC_ENTRIES_NR];
> +	enum intel_pipe_crc_source source;
> +	atomic_t slot;
> +};
> +
>  typedef struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1423,6 +1435,10 @@ typedef struct drm_i915_private {
>  	struct i915_dri1_state dri1;
>  	/* Old ums support infrastructure, same warning applies. */
>  	struct i915_ums_state ums;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES]; #endif
>  } drm_i915_private_t;
> 
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 26753b6..d2074f1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1188,6 +1188,32 @@ static void dp_aux_irq_handler(struct drm_device
> *dev)
>  	wake_up_all(&dev_priv->gmbus_wait_queue);
>  }
> 
> +#if defined(CONFIG_DEBUG_FS)
> +static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> +	struct intel_pipe_crc_entry *entry;
> +	ktime_t now;
> +	int ts, slot;
> +
> +	now = ktime_get();
> +	ts = ktime_to_us(now);
> +
> +	slot = (atomic_read(&pipe_crc->slot) + 1) %
> INTEL_PIPE_CRC_ENTRIES_NR;
> +	entry = &pipe_crc->entries[slot];
> +	entry->timestamp = ts;
> +	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
> +	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
> +	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
> +	entry->crc[3] = I915_READ(PIPE_CRC_RES_4_IVB(pipe));
> +	entry->crc[4] = I915_READ(PIPE_CRC_RES_5_IVB(pipe));
> +	atomic_set(&dev_priv->pipe_crc[pipe].slot, slot); } #else static void
> +ivb_pipe_crc_update(struct drm_device *dev, int pipe) {} #endif
> +
>  /* The RPS events need forcewake, so we add them to a work queue and mask
> their
>   * IMR bits until the work is done. Other interrupts can be processed without
>   * the work queue. */
> @@ -1366,6 +1392,15 @@ static void ivb_err_int_handler(struct drm_device
> *dev)
>  		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_C, false))
>  			DRM_DEBUG_DRIVER("Pipe C FIFO underrun\n");
> 
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_A)
> +		ivb_pipe_crc_update(dev, PIPE_A);
> +
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_B)
> +		ivb_pipe_crc_update(dev, PIPE_B);
> +
> +	if (err_int & ERR_INT_PIPE_CRC_DONE_C)
> +		ivb_pipe_crc_update(dev, PIPE_C);
> +
>  	I915_WRITE(GEN7_ERR_INT, err_int);
>  }
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 13153c3..4d01eaf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -722,8 +722,11 @@
>  #define GEN7_ERR_INT	0x44040
>  #define   ERR_INT_POISON		(1<<31)
>  #define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
> +#define   ERR_INT_PIPE_CRC_DONE_C	(1<<8)
>  #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
> +#define   ERR_INT_PIPE_CRC_DONE_B	(1<<5)
>  #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
> +#define   ERR_INT_PIPE_CRC_DONE_A	(1<<2)
>  #define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
>  #define   ERR_INT_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
> 
> @@ -1835,6 +1838,38 @@
>   * Display engine regs
>   */
> 
> +/* Pipe A CRC regs */
> +#define _PIPE_CRC_CTL_A		(dev_priv->info->display_mmio_offset +
> 0x60050)
> +#define   PIPE_CRC_ENABLE		(1 << 31)
> +#define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
> +#define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> +#define   PIPE_CRC_SOURCE_PF_IVB	(2 << 29)
> +#define _PIPE_CRC_RES_1_A_IVB	(dev_priv->info->display_mmio_offset +
> 0x60064)
> +#define _PIPE_CRC_RES_2_A_IVB	(dev_priv->info->display_mmio_offset +
> 0x60068)
> +#define _PIPE_CRC_RES_3_A_IVB	(dev_priv->info->display_mmio_offset +
> 0x6006c)
> +#define _PIPE_CRC_RES_4_A_IVB	(dev_priv->info->display_mmio_offset +
> 0x60070)
> +#define _PIPE_CRC_RES_5_A_IVB	(dev_priv->info->display_mmio_offset +
> 0x60074)
> +
> +/* Pipe B CRC regs */
> +#define _PIPE_CRC_CTL_B		(dev_priv->info->display_mmio_offset +
> 0x61050)
> +#define _PIPE_CRC_RES_1_B_IVB	(dev_priv->info->display_mmio_offset +
> 0x61064)
> +#define _PIPE_CRC_RES_2_B_IVB	(dev_priv->info->display_mmio_offset +
> 0x61068)
> +#define _PIPE_CRC_RES_3_B_IVB	(dev_priv->info->display_mmio_offset +
> 0x6106c)
> +#define _PIPE_CRC_RES_4_B_IVB	(dev_priv->info->display_mmio_offset +
> 0x61070)
> +#define _PIPE_CRC_RES_5_B_IVB	(dev_priv->info->display_mmio_offset +
> 0x61074)
> +
> +#define PIPE_CRC_CTL(pipe)	_PIPE(pipe, _PIPE_CRC_CTL_A,
> _PIPE_CRC_CTL_B)
> +#define PIPE_CRC_RES_1_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_1_A_IVB, _PIPE_CRC_RES_1_B_IVB)
> +#define PIPE_CRC_RES_2_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_2_A_IVB, _PIPE_CRC_RES_2_B_IVB)
> +#define PIPE_CRC_RES_3_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_3_A_IVB, _PIPE_CRC_RES_3_B_IVB)
> +#define PIPE_CRC_RES_4_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_4_A_IVB, _PIPE_CRC_RES_4_B_IVB)
> +#define PIPE_CRC_RES_5_IVB(pipe)	\
> +	_PIPE(pipe, _PIPE_CRC_RES_5_A_IVB, _PIPE_CRC_RES_5_B_IVB)
> +
>  /* Pipe A timing regs */
>  #define _HTOTAL_A	(dev_priv->info->display_mmio_offset + 0x60000)
>  #define _HBLANK_A	(dev_priv->info->display_mmio_offset + 0x60004)
> @@ -1857,7 +1892,6 @@
>  #define _BCLRPAT_B	(dev_priv->info->display_mmio_offset + 0x61020)
>  #define _VSYNCSHIFT_B	(dev_priv->info->display_mmio_offset + 0x61028)
> 
> -
>  #define HTOTAL(trans) _TRANSCODER(trans, _HTOTAL_A, _HTOTAL_B)
> #define HBLANK(trans) _TRANSCODER(trans, _HBLANK_A, _HBLANK_B)
> #define HSYNC(trans) _TRANSCODER(trans, _HSYNC_A, _HSYNC_B)
> --
> 1.8.3.1

[PATCH 02/16] drm/i915: Add a control file for pipe CRCs

[Damien Lespiau]

Note the "return -ENODEV;" in pipe_crc_set_source(). The ctl file is
disabled until the end of the series to be able to do incremental
improvements.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 217 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h     |   8 ++
 2 files changed, 223 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e1d45aa..0d8a9a3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -27,6 +27,7 @@
  */
 
 #include <linux/seq_file.h>
+#include <linux/ctype.h>
 #include <linux/debugfs.h>
 #include <linux/slab.h>
 #include <linux/export.h>
@@ -1742,8 +1743,8 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
 	int i;
 	int start;
 
-	if (!IS_IVYBRIDGE(dev)) {
-		seq_puts(m, "unsupported\n");
+	if (dev_priv->pipe_crc[pipe].source == INTEL_PIPE_CRC_SOURCE_NONE) {
+		seq_puts(m, "none\n");
 		return 0;
 	}
 
@@ -1762,6 +1763,217 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
 	return 0;
 }
 
+static const char *pipe_crc_sources[] = {
+	"none",
+	"plane1",
+	"plane2",
+	"pf",
+};
+
+static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
+{
+	BUILD_BUG_ON(ARRAY_SIZE(pipe_crc_sources) != INTEL_PIPE_CRC_SOURCE_MAX);
+	return pipe_crc_sources[source];
+}
+
+static int pipe_crc_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int i;
+
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		seq_printf(m, "%c %s\n", pipe_name(i),
+			   pipe_crc_source_name(dev_priv->pipe_crc[i].source));
+
+	return 0;
+}
+
+static int pipe_crc_ctl_open(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, pipe_crc_ctl_show, dev);
+}
+
+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;
+	u32 val;
+
+
+	return -ENODEV;
+
+	if (!IS_IVYBRIDGE(dev))
+		return -ENODEV;
+
+	dev_priv->pipe_crc[pipe].source = source;
+
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+		val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+		val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PF:
+		val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+	default:
+		val = 0;
+		break;
+	}
+
+	I915_WRITE(PIPE_CRC_CTL(pipe), val);
+	POSTING_READ(PIPE_CRC_CTL(pipe));
+
+	return 0;
+}
+
+/*
+ * Parse pipe CRC command strings:
+ *   command: wsp* pipe wsp+ source wsp*
+ *   pipe: (A | B | C)
+ *   source: (none | plane1 | plane2 | pf)
+ *   wsp: (#0x20 | #0x9 | #0xA)+
+ *
+ * eg.:
+ *  "A plane1"  ->  Start CRC computations on plane1 of pipe A
+ *  "A none"    ->  Stop CRC
+ */
+static int pipe_crc_ctl_tokenize(char *buf, char *words[], int max_words)
+{
+	int n_words = 0;
+
+	while (*buf) {
+		char *end;
+
+		/* skip leading white space */
+		buf = skip_spaces(buf);
+		if (!*buf)
+			break;	/* end of buffer */
+
+		/* find end of word */
+		for (end = buf; *end && !isspace(*end); end++)
+			;
+
+		if (n_words == max_words) {
+			DRM_DEBUG_DRIVER("too many words, allowed <= %d\n",
+					 max_words);
+			return -EINVAL;	/* ran out of words[] before bytes */
+		}
+
+		if (*end)
+			*end++ = '\0';
+		words[n_words++] = buf;
+		buf = end;
+	}
+
+	return n_words;
+}
+
+static int pipe_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
+{
+	const char name = buf[0];
+
+	if (name < 'A' || name >= pipe_name(I915_MAX_PIPES))
+		return -EINVAL;
+
+	*pipe = name - 'A';
+
+	return 0;
+}
+
+static int
+pipe_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *source)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
+		if (!strcmp(buf, pipe_crc_sources[i])) {
+			*source = i;
+			return 0;
+		    }
+
+	return -EINVAL;
+}
+
+static int pipe_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
+{
+#define MAX_WORDS 2
+	int n_words;
+	char *words[MAX_WORDS];
+	enum pipe pipe;
+	enum intel_pipe_crc_source source;
+
+	n_words = pipe_crc_ctl_tokenize(buf, words, MAX_WORDS);
+	if (n_words != 2) {
+		DRM_DEBUG_DRIVER("tokenize failed, a command is 2 words\n");
+		return -EINVAL;
+	}
+
+	if (pipe_crc_ctl_parse_pipe(words[0], &pipe) < 0) {
+		DRM_DEBUG_DRIVER("unknown pipe %s\n", words[0]);
+		return -EINVAL;
+	}
+
+	if (pipe_crc_ctl_parse_source(words[1], &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", words[1]);
+		return -EINVAL;
+	}
+
+	return pipe_crc_set_source(dev, pipe, source);
+}
+
+static ssize_t pipe_crc_ctl_write(struct file *file, const char __user *ubuf,
+				  size_t len, loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	struct drm_device *dev = m->private;
+	char *tmpbuf;
+	int ret;
+
+	if (len == 0)
+		return 0;
+
+	if (len > PAGE_SIZE - 1) {
+		DRM_DEBUG_DRIVER("expected <%lu bytes into pipe crc control\n",
+				 PAGE_SIZE);
+		return -E2BIG;
+	}
+
+	tmpbuf = kmalloc(len + 1, GFP_KERNEL);
+	if (!tmpbuf)
+		return -ENOMEM;
+
+	if (copy_from_user(tmpbuf, ubuf, len)) {
+		ret = -EFAULT;
+		goto out;
+	}
+	tmpbuf[len] = '\0';
+
+	ret = pipe_crc_ctl_parse(dev, tmpbuf, len);
+
+out:
+	kfree(tmpbuf);
+	if (ret < 0)
+		return ret;
+
+	*offp += len;
+	return len;
+}
+
+static const struct file_operations i915_pipe_crc_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = pipe_crc_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = pipe_crc_ctl_write
+};
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2297,6 +2509,7 @@ static struct i915_debugfs_files {
 	{"i915_gem_drop_caches", &i915_drop_caches_fops},
 	{"i915_error_state", &i915_error_state_fops},
 	{"i915_next_seqno", &i915_next_seqno_fops},
+	{"i915_pipe_crc_ctl", &i915_pipe_crc_ctl_fops},
 };
 
 int i915_debugfs_init(struct drm_minor *minor)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6855d91..0d564eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1219,6 +1219,14 @@ struct i915_package_c8 {
 	} regsave;
 };
 
+enum intel_pipe_crc_source {
+	INTEL_PIPE_CRC_SOURCE_NONE,
+	INTEL_PIPE_CRC_SOURCE_PLANE1,
+	INTEL_PIPE_CRC_SOURCE_PLANE2,
+	INTEL_PIPE_CRC_SOURCE_PF,
+	INTEL_PIPE_CRC_SOURCE_MAX,
+};
+
 struct intel_pipe_crc_entry {
 	uint32_t timestamp;
 	uint32_t crc[5];
-- 
1.8.3.1

[PATCH 03/16] drm/i915: Keep the CRC values into a circular buffer

[Damien Lespiau]

There are a few good properties to a circular buffer, for instance it
has a number of entries (before we were always dumping the full buffer).

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 20 ++++++++++++--------
 drivers/gpu/drm/i915/i915_drv.h     |  4 ++--
 drivers/gpu/drm/i915/i915_irq.c     | 19 +++++++++++++++----
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0d8a9a3..991abff 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -27,6 +27,7 @@
  */
 
 #include <linux/seq_file.h>
+#include <linux/circ_buf.h>
 #include <linux/ctype.h>
 #include <linux/debugfs.h>
 #include <linux/slab.h>
@@ -1739,25 +1740,28 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe = (enum pipe)node->info_ent->data;
-	const struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-	int i;
-	int start;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
+	int head, tail;
 
 	if (dev_priv->pipe_crc[pipe].source == INTEL_PIPE_CRC_SOURCE_NONE) {
 		seq_puts(m, "none\n");
 		return 0;
 	}
 
-	start = atomic_read(&pipe_crc->slot) + 1;
 	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
-	for (i = 0; i < INTEL_PIPE_CRC_ENTRIES_NR; i++) {
-		const struct intel_pipe_crc_entry *entry =
-			&pipe_crc->entries[(start + i) %
-					   INTEL_PIPE_CRC_ENTRIES_NR];
+	head = atomic_read(&pipe_crc->head);
+	tail = atomic_read(&pipe_crc->tail);
+
+	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
+		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
 
 		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
 			   entry->crc[0], entry->crc[1], entry->crc[2],
 			   entry->crc[3], entry->crc[4]);
+
+		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
+		tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+		atomic_set(&pipe_crc->tail, tail);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0d564eb..7a1ed3a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1232,11 +1232,11 @@ struct intel_pipe_crc_entry {
 	uint32_t crc[5];
 };
 
-#define INTEL_PIPE_CRC_ENTRIES_NR	200
+#define INTEL_PIPE_CRC_ENTRIES_NR	128
 struct intel_pipe_crc {
 	struct intel_pipe_crc_entry entries[INTEL_PIPE_CRC_ENTRIES_NR];
 	enum intel_pipe_crc_source source;
-	atomic_t slot;
+	atomic_t head, tail;
 };
 
 typedef struct drm_i915_private {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d2074f1..73d76af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -30,6 +30,7 @@
 
 #include <linux/sysrq.h>
 #include <linux/slab.h>
+#include <linux/circ_buf.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -1195,20 +1196,30 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
 	ktime_t now;
-	int ts, slot;
+	int ts, head, tail;
+
+	head = atomic_read(&pipe_crc->head);
+	tail = atomic_read(&pipe_crc->tail);
+
+	if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+		DRM_ERROR("CRC buffer overflowing\n");
+		return;
+	}
+
+	entry = &pipe_crc->entries[head];
 
 	now = ktime_get();
 	ts = ktime_to_us(now);
 
-	slot = (atomic_read(&pipe_crc->slot) + 1) % INTEL_PIPE_CRC_ENTRIES_NR;
-	entry = &pipe_crc->entries[slot];
 	entry->timestamp = ts;
 	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
 	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
 	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
 	entry->crc[3] = I915_READ(PIPE_CRC_RES_4_IVB(pipe));
 	entry->crc[4] = I915_READ(PIPE_CRC_RES_5_IVB(pipe));
-	atomic_set(&dev_priv->pipe_crc[pipe].slot, slot);
+
+	head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+	atomic_set(&pipe_crc->head, head);
 }
 #else
 static void ivb_pipe_crc_update(struct drm_device *dev, int pipe) {}
-- 
1.8.3.1

[PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
 drivers/gpu/drm/i915/i915_drv.h     | 2 +-
 drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 991abff..58c6fd4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
 		return 0;
 	}
 
-	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
+	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
 	head = atomic_read(&pipe_crc->head);
 	tail = atomic_read(&pipe_crc->tail);
 
 	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
 		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
 
-		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
+		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
 			   entry->crc[0], entry->crc[1], entry->crc[2],
 			   entry->crc[3], entry->crc[4]);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a1ed3a..cd87919 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
 };
 
 struct intel_pipe_crc_entry {
-	uint32_t timestamp;
+	uint32_t frame;
 	uint32_t crc[5];
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 73d76af..0b21828 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
-	ktime_t now;
-	int ts, head, tail;
+	int head, tail;
 
 	head = atomic_read(&pipe_crc->head);
 	tail = atomic_read(&pipe_crc->tail);
@@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
 
 	entry = &pipe_crc->entries[head];
 
-	now = ktime_get();
-	ts = ktime_to_us(now);
-
-	entry->timestamp = ts;
+	entry->frame = I915_READ(PIPEFRAME(pipe));
 	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
 	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
 	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
-- 
1.8.3.1

Re: [PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Ville Syrjälä]

On Tue, Oct 15, 2013 at 06:55:30PM +0100, Damien Lespiau wrote:
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
>  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
>  drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
>  3 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 991abff..58c6fd4 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
>  		return 0;
>  	}
>  
> -	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> +	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
>  	head = atomic_read(&pipe_crc->head);
>  	tail = atomic_read(&pipe_crc->tail);
>  
>  	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
>  		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
>  
> -		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> +		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
>  			   entry->crc[0], entry->crc[1], entry->crc[2],
>  			   entry->crc[3], entry->crc[4]);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a1ed3a..cd87919 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
>  };
>  
>  struct intel_pipe_crc_entry {
> -	uint32_t timestamp;
> +	uint32_t frame;
>  	uint32_t crc[5];
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 73d76af..0b21828 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
>  	struct intel_pipe_crc_entry *entry;
> -	ktime_t now;
> -	int ts, head, tail;
> +	int head, tail;
>  
>  	head = atomic_read(&pipe_crc->head);
>  	tail = atomic_read(&pipe_crc->tail);
> @@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
>  
>  	entry = &pipe_crc->entries[head];
>  
> -	now = ktime_get();
> -	ts = ktime_to_us(now);
> -
> -	entry->timestamp = ts;
> +	entry->frame = I915_READ(PIPEFRAME(pipe));

BTW that's the wrong register for ctg+. It should be PIPE_FRMCOUNT_GM45.
But it would be better if you just call
dev->driver->get_vblank_counter(). Then you get the correct answer for
everything except gen2, and for gen2 we could implement a software
frame counter if need be.

>  	entry->crc[0] = I915_READ(PIPE_CRC_RES_1_IVB(pipe));
>  	entry->crc[1] = I915_READ(PIPE_CRC_RES_2_IVB(pipe));
>  	entry->crc[2] = I915_READ(PIPE_CRC_RES_3_IVB(pipe));
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Daniel Vetter]

On Wed, Oct 16, 2013 at 04:29:32PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 15, 2013 at 06:55:30PM +0100, Damien Lespiau wrote:
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> >  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
> >  drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
> >  3 files changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 991abff..58c6fd4 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
> >  		return 0;
> >  	}
> >  
> > -	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > +	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
> >  	head = atomic_read(&pipe_crc->head);
> >  	tail = atomic_read(&pipe_crc->tail);
> >  
> >  	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
> >  		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
> >  
> > -		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> > +		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
> >  			   entry->crc[0], entry->crc[1], entry->crc[2],
> >  			   entry->crc[3], entry->crc[4]);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7a1ed3a..cd87919 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
> >  };
> >  
> >  struct intel_pipe_crc_entry {
> > -	uint32_t timestamp;
> > +	uint32_t frame;
> >  	uint32_t crc[5];
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 73d76af..0b21828 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> >  	struct intel_pipe_crc_entry *entry;
> > -	ktime_t now;
> > -	int ts, head, tail;
> > +	int head, tail;
> >  
> >  	head = atomic_read(&pipe_crc->head);
> >  	tail = atomic_read(&pipe_crc->tail);
> > @@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> >  
> >  	entry = &pipe_crc->entries[head];
> >  
> > -	now = ktime_get();
> > -	ts = ktime_to_us(now);
> > -
> > -	entry->timestamp = ts;
> > +	entry->frame = I915_READ(PIPEFRAME(pipe));
> 
> BTW that's the wrong register for ctg+. It should be PIPE_FRMCOUNT_GM45.
> But it would be better if you just call
> dev->driver->get_vblank_counter(). Then you get the correct answer for
> everything except gen2, and for gen2 we could implement a software
> frame counter if need be.

Hm, maybe we should even cobble it out of the drm vblank support code, to
make sure that the vblank frame numbers from the crc debugfs align with
timestamps for pageflips. But this is definitely a good idea. And afaik
gen2 doesn't have crc support (not even gen3 iirc). I'll do this in my
bikeshed series.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Ville Syrjälä]

On Wed, Oct 16, 2013 at 03:51:40PM +0200, Daniel Vetter wrote:
> On Wed, Oct 16, 2013 at 04:29:32PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 15, 2013 at 06:55:30PM +0100, Damien Lespiau wrote:
> > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> > >  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
> > >  drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
> > >  3 files changed, 5 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 991abff..58c6fd4 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
> > >  		return 0;
> > >  	}
> > >  
> > > -	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > > +	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > >  	head = atomic_read(&pipe_crc->head);
> > >  	tail = atomic_read(&pipe_crc->tail);
> > >  
> > >  	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
> > >  		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
> > >  
> > > -		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> > > +		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
> > >  			   entry->crc[0], entry->crc[1], entry->crc[2],
> > >  			   entry->crc[3], entry->crc[4]);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7a1ed3a..cd87919 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
> > >  };
> > >  
> > >  struct intel_pipe_crc_entry {
> > > -	uint32_t timestamp;
> > > +	uint32_t frame;
> > >  	uint32_t crc[5];
> > >  };
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 73d76af..0b21828 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > >  	struct intel_pipe_crc_entry *entry;
> > > -	ktime_t now;
> > > -	int ts, head, tail;
> > > +	int head, tail;
> > >  
> > >  	head = atomic_read(&pipe_crc->head);
> > >  	tail = atomic_read(&pipe_crc->tail);
> > > @@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> > >  
> > >  	entry = &pipe_crc->entries[head];
> > >  
> > > -	now = ktime_get();
> > > -	ts = ktime_to_us(now);
> > > -
> > > -	entry->timestamp = ts;
> > > +	entry->frame = I915_READ(PIPEFRAME(pipe));
> > 
> > BTW that's the wrong register for ctg+. It should be PIPE_FRMCOUNT_GM45.
> > But it would be better if you just call
> > dev->driver->get_vblank_counter(). Then you get the correct answer for
> > everything except gen2, and for gen2 we could implement a software
> > frame counter if need be.
> 
> Hm, maybe we should even cobble it out of the drm vblank support code, to
> make sure that the vblank frame numbers from the crc debugfs align with
> timestamps for pageflips. But this is definitely a good idea. And afaik
> gen2 doesn't have crc support (not even gen3 iirc). I'll do this in my
> bikeshed series.

I see crc registers in both gen2 and 3 bspec.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Ville Syrjälä]

On Wed, Oct 16, 2013 at 03:51:40PM +0200, Daniel Vetter wrote:
> On Wed, Oct 16, 2013 at 04:29:32PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 15, 2013 at 06:55:30PM +0100, Damien Lespiau wrote:
> > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> > >  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
> > >  drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
> > >  3 files changed, 5 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 991abff..58c6fd4 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
> > >  		return 0;
> > >  	}
> > >  
> > > -	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > > +	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > >  	head = atomic_read(&pipe_crc->head);
> > >  	tail = atomic_read(&pipe_crc->tail);
> > >  
> > >  	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
> > >  		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
> > >  
> > > -		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> > > +		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
> > >  			   entry->crc[0], entry->crc[1], entry->crc[2],
> > >  			   entry->crc[3], entry->crc[4]);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7a1ed3a..cd87919 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
> > >  };
> > >  
> > >  struct intel_pipe_crc_entry {
> > > -	uint32_t timestamp;
> > > +	uint32_t frame;
> > >  	uint32_t crc[5];
> > >  };
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 73d76af..0b21828 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > >  	struct intel_pipe_crc_entry *entry;
> > > -	ktime_t now;
> > > -	int ts, head, tail;
> > > +	int head, tail;
> > >  
> > >  	head = atomic_read(&pipe_crc->head);
> > >  	tail = atomic_read(&pipe_crc->tail);
> > > @@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> > >  
> > >  	entry = &pipe_crc->entries[head];
> > >  
> > > -	now = ktime_get();
> > > -	ts = ktime_to_us(now);
> > > -
> > > -	entry->timestamp = ts;
> > > +	entry->frame = I915_READ(PIPEFRAME(pipe));
> > 
> > BTW that's the wrong register for ctg+. It should be PIPE_FRMCOUNT_GM45.
> > But it would be better if you just call
> > dev->driver->get_vblank_counter(). Then you get the correct answer for
> > everything except gen2, and for gen2 we could implement a software
> > frame counter if need be.
> 
> Hm, maybe we should even cobble it out of the drm vblank support code, to
> make sure that the vblank frame numbers from the crc debugfs align with
> timestamps for pageflips.

Well the frame numbers reported to userspace by vblank/page flip code
are based on a software counter maintained by the drm vblank code.
.get_vblank_counter() is the real hardware counter. I guess you mean
that we should try to report the software counter for userspace here
as well. Sounds reasonable.

-- 
Ville Syrjälä
Intel OTC

[PATCH 05/16] drm/i915: Make switching to the same CRC source a no-op

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 58c6fd4..8c750d5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1804,6 +1804,7 @@ 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];
 	u32 val;
 
 
@@ -1812,7 +1813,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	if (!IS_IVYBRIDGE(dev))
 		return -ENODEV;
 
-	dev_priv->pipe_crc[pipe].source = source;
+	if (pipe_crc->source == source)
+		return 0;
+
+	pipe_crc->source = source;
 
 	switch (source) {
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
-- 
1.8.3.1

[PATCH 06/16] drm/i915: Enforce going back to none before changing CRC source

[Damien Lespiau]

This way we can have some init/fini code on those transitions.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8c750d5..787c50d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1816,6 +1816,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	if (pipe_crc->source == source)
 		return 0;
 
+	/* forbid changing the source without going back to 'none' */
+	if (pipe_crc->source && source)
+		return -EINVAL;
+
 	pipe_crc->source = source;
 
 	switch (source) {
-- 
1.8.3.1

[PATCH 07/16] drm/i915: Empty the circular buffer when asked for a new source

[Damien Lespiau]

So we don't read out stale CRCs from a previous run left in the buffer.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 787c50d..ec9151a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1820,6 +1820,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	if (pipe_crc->source && source)
 		return -EINVAL;
 
+	/* none -> real source transition */
+	if (source) {
+		atomic_set(&pipe_crc->head, 0);
+		atomic_set(&pipe_crc->tail, 0);
+	}
+
 	pipe_crc->source = source;
 
 	switch (source) {
-- 
1.8.3.1

[PATCH 08/16] drm/i915: Dynamically allocate the CRC circular buffer

[Damien Lespiau]

So we don't eat that memory when not needed.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ec9151a..53a3f22 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1822,6 +1822,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		pipe_crc->entries = kzalloc(sizeof(*pipe_crc->entries) *
+					    INTEL_PIPE_CRC_ENTRIES_NR,
+					    GFP_KERNEL);
+		if (!pipe_crc->entries)
+			return -ENOMEM;
+
 		atomic_set(&pipe_crc->head, 0);
 		atomic_set(&pipe_crc->tail, 0);
 	}
@@ -1847,6 +1853,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	I915_WRITE(PIPE_CRC_CTL(pipe), val);
 	POSTING_READ(PIPE_CRC_CTL(pipe));
 
+	/* real source -> none transition */
+	if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+		kfree(pipe_crc->entries);
+		pipe_crc->entries = NULL;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cd87919..5171a69 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,7 +1234,7 @@ struct intel_pipe_crc_entry {
 
 #define INTEL_PIPE_CRC_ENTRIES_NR	128
 struct intel_pipe_crc {
-	struct intel_pipe_crc_entry entries[INTEL_PIPE_CRC_ENTRIES_NR];
+	struct intel_pipe_crc_entry *entries;
 	enum intel_pipe_crc_source source;
 	atomic_t head, tail;
 };
-- 
1.8.3.1

[PATCH 09/16] drm/i915: Generalize the CRC command format for future work

[Damien Lespiau]

Let's move from writing 'A plane1' to 'pipe A plane1' to
i915_pipe_crc_ctl. This will allow us to extend the interface to
transcoders or DDIs in the future.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 56 ++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 53a3f22..c609783 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1864,14 +1864,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 /*
  * Parse pipe CRC command strings:
- *   command: wsp* pipe wsp+ source wsp*
- *   pipe: (A | B | C)
+ *   command: wsp* object wsp+ name wsp+ source wsp*
+ *   object: 'pipe'
+ *   name: (A | B | C)
  *   source: (none | plane1 | plane2 | pf)
  *   wsp: (#0x20 | #0x9 | #0xA)+
  *
  * eg.:
- *  "A plane1"  ->  Start CRC computations on plane1 of pipe A
- *  "A none"    ->  Stop CRC
+ *  "pipe A plane1"  ->  Start CRC computations on plane1 of pipe A
+ *  "pipe A none"    ->  Stop CRC
  */
 static int pipe_crc_ctl_tokenize(char *buf, char *words[], int max_words)
 {
@@ -1904,6 +1905,28 @@ static int pipe_crc_ctl_tokenize(char *buf, char *words[], int max_words)
 	return n_words;
 }
 
+enum intel_pipe_crc_object {
+	PIPE_CRC_OBJECT_PIPE,
+};
+
+static const char *pipe_crc_objects[] = {
+	"pipe",
+};
+
+static int
+pipe_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *object)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pipe_crc_objects); i++)
+		if (!strcmp(buf, pipe_crc_objects[i])) {
+			*object = i;
+			return 0;
+		    }
+
+	return -EINVAL;
+}
+
 static int pipe_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
 {
 	const char name = buf[0];
@@ -1932,25 +1955,32 @@ pipe_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *source)
 
 static int pipe_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
 {
-#define MAX_WORDS 2
+#define N_WORDS 3
 	int n_words;
-	char *words[MAX_WORDS];
+	char *words[N_WORDS];
 	enum pipe pipe;
+	enum intel_pipe_crc_object object;
 	enum intel_pipe_crc_source source;
 
-	n_words = pipe_crc_ctl_tokenize(buf, words, MAX_WORDS);
-	if (n_words != 2) {
-		DRM_DEBUG_DRIVER("tokenize failed, a command is 2 words\n");
+	n_words = pipe_crc_ctl_tokenize(buf, words, N_WORDS);
+	if (n_words != N_WORDS) {
+		DRM_DEBUG_DRIVER("tokenize failed, a command is %d words\n",
+				 N_WORDS);
+		return -EINVAL;
+	}
+
+	if (pipe_crc_ctl_parse_object(words[0], &object) < 0) {
+		DRM_DEBUG_DRIVER("unknown object %s\n", words[0]);
 		return -EINVAL;
 	}
 
-	if (pipe_crc_ctl_parse_pipe(words[0], &pipe) < 0) {
-		DRM_DEBUG_DRIVER("unknown pipe %s\n", words[0]);
+	if (pipe_crc_ctl_parse_pipe(words[1], &pipe) < 0) {
+		DRM_DEBUG_DRIVER("unknown pipe %s\n", words[1]);
 		return -EINVAL;
 	}
 
-	if (pipe_crc_ctl_parse_source(words[1], &source) < 0) {
-		DRM_DEBUG_DRIVER("unknown source %s\n", words[1]);
+	if (pipe_crc_ctl_parse_source(words[2], &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", words[2]);
 		return -EINVAL;
 	}
 
-- 
1.8.3.1

[PATCH 10/16] drm/i915: Rename i915_pipe_crc_ctl to i915_display_crc_ctl

[Damien Lespiau]

In the same spirit than:

    drm/i915: Generalize the CRC command format for future work

    Let's move from writing 'A plane1' to 'pipe A plane1' to
    i915_pipe_crc_ctl. This will allow us to extend the interface to
    transcoders or DDIs in the future.

Let's rename the CRC control file to be more generic.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 42 ++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c609783..471c258 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1780,7 +1780,7 @@ static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
 	return pipe_crc_sources[source];
 }
 
-static int pipe_crc_ctl_show(struct seq_file *m, void *data)
+static int display_crc_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1793,11 +1793,11 @@ static int pipe_crc_ctl_show(struct seq_file *m, void *data)
 	return 0;
 }
 
-static int pipe_crc_ctl_open(struct inode *inode, struct file *file)
+static int display_crc_ctl_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *dev = inode->i_private;
 
-	return single_open(file, pipe_crc_ctl_show, dev);
+	return single_open(file, display_crc_ctl_show, dev);
 }
 
 static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
@@ -1874,7 +1874,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
  *  "pipe A plane1"  ->  Start CRC computations on plane1 of pipe A
  *  "pipe A none"    ->  Stop CRC
  */
-static int pipe_crc_ctl_tokenize(char *buf, char *words[], int max_words)
+static int display_crc_ctl_tokenize(char *buf, char *words[], int max_words)
 {
 	int n_words = 0;
 
@@ -1914,20 +1914,20 @@ static const char *pipe_crc_objects[] = {
 };
 
 static int
-pipe_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *object)
+display_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *o)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(pipe_crc_objects); i++)
 		if (!strcmp(buf, pipe_crc_objects[i])) {
-			*object = i;
+			*o = i;
 			return 0;
 		    }
 
 	return -EINVAL;
 }
 
-static int pipe_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
+static int display_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
 {
 	const char name = buf[0];
 
@@ -1940,20 +1940,20 @@ static int pipe_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
 }
 
 static int
-pipe_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *source)
+display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
 		if (!strcmp(buf, pipe_crc_sources[i])) {
-			*source = i;
+			*s = i;
 			return 0;
 		    }
 
 	return -EINVAL;
 }
 
-static int pipe_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
+static int display_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
 {
 #define N_WORDS 3
 	int n_words;
@@ -1962,24 +1962,24 @@ static int pipe_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
 	enum intel_pipe_crc_object object;
 	enum intel_pipe_crc_source source;
 
-	n_words = pipe_crc_ctl_tokenize(buf, words, N_WORDS);
+	n_words = display_crc_ctl_tokenize(buf, words, N_WORDS);
 	if (n_words != N_WORDS) {
 		DRM_DEBUG_DRIVER("tokenize failed, a command is %d words\n",
 				 N_WORDS);
 		return -EINVAL;
 	}
 
-	if (pipe_crc_ctl_parse_object(words[0], &object) < 0) {
+	if (display_crc_ctl_parse_object(words[0], &object) < 0) {
 		DRM_DEBUG_DRIVER("unknown object %s\n", words[0]);
 		return -EINVAL;
 	}
 
-	if (pipe_crc_ctl_parse_pipe(words[1], &pipe) < 0) {
+	if (display_crc_ctl_parse_pipe(words[1], &pipe) < 0) {
 		DRM_DEBUG_DRIVER("unknown pipe %s\n", words[1]);
 		return -EINVAL;
 	}
 
-	if (pipe_crc_ctl_parse_source(words[2], &source) < 0) {
+	if (display_crc_ctl_parse_source(words[2], &source) < 0) {
 		DRM_DEBUG_DRIVER("unknown source %s\n", words[2]);
 		return -EINVAL;
 	}
@@ -1987,8 +1987,8 @@ static int pipe_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
 	return pipe_crc_set_source(dev, pipe, source);
 }
 
-static ssize_t pipe_crc_ctl_write(struct file *file, const char __user *ubuf,
-				  size_t len, loff_t *offp)
+static ssize_t display_crc_ctl_write(struct file *file, const char __user *ubuf,
+				     size_t len, loff_t *offp)
 {
 	struct seq_file *m = file->private_data;
 	struct drm_device *dev = m->private;
@@ -2014,7 +2014,7 @@ static ssize_t pipe_crc_ctl_write(struct file *file, const char __user *ubuf,
 	}
 	tmpbuf[len] = '\0';
 
-	ret = pipe_crc_ctl_parse(dev, tmpbuf, len);
+	ret = display_crc_ctl_parse(dev, tmpbuf, len);
 
 out:
 	kfree(tmpbuf);
@@ -2025,13 +2025,13 @@ out:
 	return len;
 }
 
-static const struct file_operations i915_pipe_crc_ctl_fops = {
+static const struct file_operations i915_display_crc_ctl_fops = {
 	.owner = THIS_MODULE,
-	.open = pipe_crc_ctl_open,
+	.open = display_crc_ctl_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = single_release,
-	.write = pipe_crc_ctl_write
+	.write = display_crc_ctl_write
 };
 
 static int
@@ -2569,7 +2569,7 @@ static struct i915_debugfs_files {
 	{"i915_gem_drop_caches", &i915_drop_caches_fops},
 	{"i915_error_state", &i915_error_state_fops},
 	{"i915_next_seqno", &i915_next_seqno_fops},
-	{"i915_pipe_crc_ctl", &i915_pipe_crc_ctl_fops},
+	{"i915_display_crc_ctl", &i915_display_crc_ctl_fops},
 };
 
 int i915_debugfs_init(struct drm_minor *minor)
-- 
1.8.3.1

[PATCH 11/16] drm/i915: Warn if we receive an interrupt after freeing the buffer

[Damien Lespiau]

This shouldn't happen as the buffer is freed after disable pipe CRCs,
but better be safe than sorry.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b21828..b201a21 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1197,6 +1197,11 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
 	struct intel_pipe_crc_entry *entry;
 	int head, tail;
 
+	if (!pipe_crc->entries) {
+		DRM_ERROR("spurious interrupt\n");
+		return;
+	}
+
 	head = atomic_read(&pipe_crc->head);
 	tail = atomic_read(&pipe_crc->tail);
 
-- 
1.8.3.1

[PATCH 12/16] drm/i915: Add log messages when CRCs collection is started/stopped

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 471c258..dee85d7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1822,6 +1822,9 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
+				 pipe_name(pipe), pipe_crc_source_name(source));
+
 		pipe_crc->entries = kzalloc(sizeof(*pipe_crc->entries) *
 					    INTEL_PIPE_CRC_ENTRIES_NR,
 					    GFP_KERNEL);
@@ -1855,6 +1858,9 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* real source -> none transition */
 	if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+		DRM_DEBUG_DRIVER("stopping CRCs for pipe %c\n",
+				 pipe_name(pipe));
+
 		kfree(pipe_crc->entries);
 		pipe_crc->entries = NULL;
 	}
-- 
1.8.3.1

[PATCH 13/16] drm/i915: Move drm_add_fake_info_node() higher in the file

[Damien Lespiau]

Following commit needs drm_add_fake_info_node() higher in the file to
avoid having a forward declaration. Move this helper near the top of the
file.

This also makes the next commit diff a bit easier to review.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 52 ++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index dee85d7..baa2e43 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -53,6 +53,32 @@ static const char *yesno(int v)
 	return v ? "yes" : "no";
 }
 
+/* As the drm_debugfs_init() routines are called before dev->dev_private is
+ * allocated we need to hook into the minor for release. */
+static int
+drm_add_fake_info_node(struct drm_minor *minor,
+		       struct dentry *ent,
+		       const void *key)
+{
+	struct drm_info_node *node;
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (node == NULL) {
+		debugfs_remove(ent);
+		return -ENOMEM;
+	}
+
+	node->minor = minor;
+	node->dent = ent;
+	node->info_ent = (void *) key;
+
+	mutex_lock(&minor->debugfs_lock);
+	list_add(&node->list, &minor->debugfs_list);
+	mutex_unlock(&minor->debugfs_lock);
+
+	return 0;
+}
+
 static int i915_capabilities(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -2425,32 +2451,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
 			i915_cache_sharing_get, i915_cache_sharing_set,
 			"%llu\n");
 
-/* As the drm_debugfs_init() routines are called before dev->dev_private is
- * allocated we need to hook into the minor for release. */
-static int
-drm_add_fake_info_node(struct drm_minor *minor,
-		       struct dentry *ent,
-		       const void *key)
-{
-	struct drm_info_node *node;
-
-	node = kmalloc(sizeof(*node), GFP_KERNEL);
-	if (node == NULL) {
-		debugfs_remove(ent);
-		return -ENOMEM;
-	}
-
-	node->minor = minor;
-	node->dent = ent;
-	node->info_ent = (void *) key;
-
-	mutex_lock(&minor->debugfs_lock);
-	list_add(&node->list, &minor->debugfs_list);
-	mutex_unlock(&minor->debugfs_lock);
-
-	return 0;
-}
-
 static int i915_forcewake_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *dev = inode->i_private;
-- 
1.8.3.1

[PATCH 14/16] drm/i915: Implement blocking read for pipe CRC files

[Damien Lespiau]

seq_file is not quite the right interface for these ones. We have a
circular buffer with a new entry per vblank on one side and a process
wanting to dequeue the CRC with a read().

It's quite racy to wait for vblank in user land and then try to read a
pipe_crc file, sometimes the CRC interrupt hasn't been fired and we end
up with an EOF.

So, let's have the read on the pipe_crc file block until the interrupt
gives us a new entry. At that point we can wake the reading process.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 164 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_dma.c     |   2 +
 drivers/gpu/drm/i915/i915_drv.h     |   6 ++
 drivers/gpu/drm/i915/i915_irq.c     |   2 +
 4 files changed, 155 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index baa2e43..5137f8f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1760,37 +1760,138 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
-static int i915_pipe_crc(struct seq_file *m, void *data)
+struct pipe_crc_info {
+	const char *name;
+	struct drm_device *dev;
+	enum pipe pipe;
+};
+
+static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
+{
+	filep->private_data = inode->i_private;
+
+	return 0;
+}
+
+static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
+{
+	return 0;
+}
+
+/* (6 fields, 8 chars each, space separated (5) + '\n') */
+#define PIPE_CRC_LINE_LEN	(6 * 8 + 5 + 1)
+/* account for \'0' */
+#define PIPE_CRC_BUFFER_LEN	(PIPE_CRC_LINE_LEN + 1)
+
+static int pipe_crc_data_count(struct intel_pipe_crc *pipe_crc)
 {
-	struct drm_info_node *node = (struct drm_info_node *) m->private;
-	struct drm_device *dev = node->minor->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe = (enum pipe)node->info_ent->data;
-	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	int head, tail;
 
-	if (dev_priv->pipe_crc[pipe].source == INTEL_PIPE_CRC_SOURCE_NONE) {
-		seq_puts(m, "none\n");
+	head = atomic_read(&pipe_crc->head);
+	tail = atomic_read(&pipe_crc->tail);
+
+	return CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR);
+}
+
+static ssize_t
+i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
+		   loff_t *pos)
+{
+	struct pipe_crc_info *info = filep->private_data;
+	struct drm_device *dev = info->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	char buf[PIPE_CRC_BUFFER_LEN];
+	int head, tail, n_entries, n;
+	ssize_t bytes_read;
+
+	/*
+	 * Don't allow user space to provide buffers not big enough to hold
+	 * a line of data.
+	 */
+	if (count < PIPE_CRC_LINE_LEN)
+		return -EINVAL;
+
+	if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
 		return 0;
+
+	/* nothing to read */
+	while (pipe_crc_data_count(pipe_crc) == 0) {
+		if (filep->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		if (wait_event_interruptible(pipe_crc->wq,
+					     pipe_crc_data_count(pipe_crc)))
+			 return -ERESTARTSYS;
 	}
 
-	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
+	/* We now have one or more entries to read */
 	head = atomic_read(&pipe_crc->head);
 	tail = atomic_read(&pipe_crc->tail);
-
-	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
+	n_entries = min((size_t)CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR),
+			count / PIPE_CRC_LINE_LEN);
+	bytes_read = 0;
+	n = 0;
+	do {
 		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
+		int ret;
 
-		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
-			   entry->crc[0], entry->crc[1], entry->crc[2],
-			   entry->crc[3], entry->crc[4]);
+		bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
+				       "%8u %8x %8x %8x %8x %8x\n",
+				       entry->frame, entry->crc[0],
+				       entry->crc[1], entry->crc[2],
+				       entry->crc[3], entry->crc[4]);
+
+		ret = copy_to_user(user_buf + n * PIPE_CRC_LINE_LEN,
+				   buf, PIPE_CRC_LINE_LEN);
+		if (ret == PIPE_CRC_LINE_LEN)
+			return -EFAULT;
 
 		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
 		tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
 		atomic_set(&pipe_crc->tail, tail);
-	}
+		n++;
+	} while (--n_entries);
 
-	return 0;
+	return bytes_read;
+}
+
+static const struct file_operations i915_pipe_crc_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_pipe_crc_open,
+	.read = i915_pipe_crc_read,
+	.release = i915_pipe_crc_release,
+};
+
+static struct pipe_crc_info i915_pipe_crc_data[I915_MAX_PIPES] = {
+	{
+		.name = "i915_pipe_A_crc",
+		.pipe = PIPE_A,
+	},
+	{
+		.name = "i915_pipe_B_crc",
+		.pipe = PIPE_B,
+	},
+	{
+		.name = "i915_pipe_C_crc",
+		.pipe = PIPE_C,
+	},
+};
+
+static int i915_pipe_crc_create(struct dentry *root, struct drm_minor *minor,
+				enum pipe pipe)
+{
+	struct drm_device *dev = minor->dev;
+	struct dentry *ent;
+	struct pipe_crc_info *info = &i915_pipe_crc_data[pipe];
+
+	info->dev = dev;
+	ent = debugfs_create_file(info->name, S_IRUGO, root, info,
+				  &i915_pipe_crc_fops);
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);
+
+	return drm_add_fake_info_node(minor, ent, info);
 }
 
 static const char *pipe_crc_sources[] = {
@@ -2555,9 +2656,6 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
-	{"i915_pipe_A_crc", i915_pipe_crc, 0, (void *)PIPE_A},
-	{"i915_pipe_B_crc", i915_pipe_crc, 0, (void *)PIPE_B},
-	{"i915_pipe_C_crc", i915_pipe_crc, 0, (void *)PIPE_C},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
@@ -2578,6 +2676,18 @@ static struct i915_debugfs_files {
 	{"i915_display_crc_ctl", &i915_display_crc_ctl_fops},
 };
 
+void intel_display_crc_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int i;
+
+	for (i = 0; i < INTEL_INFO(dev)->num_pipes; i++) {
+		struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[i];
+
+		init_waitqueue_head(&pipe_crc->wq);
+	}
+}
+
 int i915_debugfs_init(struct drm_minor *minor)
 {
 	int ret, i;
@@ -2586,6 +2696,12 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
+		ret = i915_pipe_crc_create(minor->debugfs_root, minor, i);
+		if (ret)
+			return ret;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
 		ret = i915_debugfs_create(minor->debugfs_root, minor,
 					  i915_debugfs_files[i].name,
@@ -2601,12 +2717,22 @@ int i915_debugfs_init(struct drm_minor *minor)
 
 void i915_debugfs_cleanup(struct drm_minor *minor)
 {
+	struct drm_device *dev = minor->dev;
 	int i;
 
 	drm_debugfs_remove_files(i915_debugfs_list,
 				 I915_DEBUGFS_ENTRIES, minor);
+
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
 				 1, minor);
+
+	for (i = 0; i < INTEL_INFO(dev)->num_pipes; i++) {
+		struct drm_info_list *info_list =
+			(struct drm_info_list *)&i915_pipe_crc_data[i];
+
+		drm_debugfs_remove_files(info_list, 1, minor);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
 		struct drm_info_list *info_list =
 			(struct drm_info_list *) i915_debugfs_files[i].fops;
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b8bc887..4a57a7c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1501,6 +1501,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
 	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
 
+	intel_display_crc_init(dev);
+
 	i915_dump_device_info(dev_priv);
 
 	/* Not all pre-production machines fall into this category, only the
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5171a69..e16cf1e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1237,6 +1237,7 @@ struct intel_pipe_crc {
 	struct intel_pipe_crc_entry *entries;
 	enum intel_pipe_crc_source source;
 	atomic_t head, tail;
+	wait_queue_head_t wq;
 };
 
 typedef struct drm_i915_private {
@@ -2231,6 +2232,11 @@ int i915_verify_lists(struct drm_device *dev);
 /* i915_debugfs.c */
 int i915_debugfs_init(struct drm_minor *minor);
 void i915_debugfs_cleanup(struct drm_minor *minor);
+#if defined(CONFIG_DEBUG_FS)
+void intel_display_crc_init(struct drm_device *dev);
+#else
+void intel_display_crc_init(struct drm_device *dev) {}
+#endif
 
 /* i915_gpu_error.c */
 __printf(2, 3)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b201a21..b2be057 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1221,6 +1221,8 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
 
 	head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
 	atomic_set(&pipe_crc->head, head);
+
+	wake_up_interruptible(&pipe_crc->wq);
 }
 #else
 static void ivb_pipe_crc_update(struct drm_device *dev, int pipe) {}
-- 
1.8.3.1

[PATCH 15/16] drm/i915: Only one open() allowed on pipe CRC result files

[Damien Lespiau]

It doesn't really make sense to have two processes dequeueing the CRC
values at the same time. Forbid that usage.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 16 ++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5137f8f..826ebce 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1768,6 +1768,15 @@ struct pipe_crc_info {
 
 static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
 {
+	struct pipe_crc_info *info = inode->i_private;
+	struct drm_i915_private *dev_priv = info->dev->dev_private;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+
+	if (!atomic_dec_and_test(&pipe_crc->available)) {
+		atomic_inc(&pipe_crc->available);
+		return -EBUSY; /* already open */
+	}
+
 	filep->private_data = inode->i_private;
 
 	return 0;
@@ -1775,6 +1784,12 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
 
 static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 {
+	struct pipe_crc_info *info = inode->i_private;
+	struct drm_i915_private *dev_priv = info->dev->dev_private;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+
+	atomic_inc(&pipe_crc->available); /* release the device */
+
 	return 0;
 }
 
@@ -2684,6 +2699,7 @@ void intel_display_crc_init(struct drm_device *dev)
 	for (i = 0; i < INTEL_INFO(dev)->num_pipes; i++) {
 		struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[i];
 
+		atomic_set(&pipe_crc->available, 1);
 		init_waitqueue_head(&pipe_crc->wq);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e16cf1e..3206358 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,6 +1234,7 @@ struct intel_pipe_crc_entry {
 
 #define INTEL_PIPE_CRC_ENTRIES_NR	128
 struct intel_pipe_crc {
+	atomic_t available;		/* exclusive access to the device */
 	struct intel_pipe_crc_entry *entries;
 	enum intel_pipe_crc_source source;
 	atomic_t head, tail;
-- 
1.8.3.1

[PATCH 16/16] drm/i915: Enable pipe CRCs

[Damien Lespiau]

It's time to declare them ready. Unleash the beast.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 826ebce..d1674b6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1949,9 +1949,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	u32 val;
 
-
-	return -ENODEV;
-
 	if (!IS_IVYBRIDGE(dev))
 		return -ENODEV;
 
-- 
1.8.3.1

Re: Pipe CRCs v1

[Daniel Vetter]

On Tue, Oct 15, 2013 at 06:55:26PM +0100, Damien Lespiau wrote:
> This series exposes the pipe CRCs on ivybridge through debugfs. It's based on
> the initial work from Shuang He, with some improvements to have a nice user
> space API.
> 
> There are several points in the display pipeline where CRCs can be computed
> on the bits flowing there. For instance, it's usually possible to compute
> the CRCs of the primary plane, the sprite plane or the CRCs of the bits
> after the panel fitter (collectively called pipe CRCs).
> 
> An intel-gpu-tools series will follow with helpers to use the feature from
> tests and basic testing.
> 
> Further work items:
>   * make it work on other platforms
>   * expose other CRCs than just the pipe CRCs (transcoders, ddi, ...)
>   * implement poll() for the result files

I like this and so went ahead and merged it right away. A bit of CRC
support is much better than none and the tests should make sure that
things actually work somewhat.

Polishing that imo still needs to happen:
- Roll out on other platforms. Especially vlv since there we actually want
  to have good sprite support.

- Add an "all" alias or something similar which just picks a crc source
  take takes the pixels after all sprite/cursor/plane blending, csc and
  gamma ramps have been applied. For ivb that seems to be the "pf" source.
  To make sure it works we'd need a small testcase that moves
  cursors/sprites/planes and changes gamma to make sure we cover all these
  things. Then we could finally have automated testcases for checking all
  the restore code in our resume paths (or dpms on and all that stuff).

- atomic_t is weakly ordered, even on x86 (since it doesn't even have).
  Also, writing a correct lockless ringbuffer is Damn Hard (tm). I don't
  see any need to massive performance issues with this, so we need to
  switch to normal mutex locking (with a spinlock for the ringbuffer
  itself to allow the irq handler to fill it).

- Use it all over the place in igt tests ...

Thanks a lot for doing this.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch