Re: [PATCH v2 2/3] drm: Add API for capturing frame CRCs

[Ville Syrjälä]

On Thu, Jul 07, 2016 at 05:06:08PM +0200, Tomeu Vizoso wrote:
> Adds files and directories to debugfs for controlling and reading frame
> CRCs, per CRTC:
> 
> dri/0/crtc-0/crc
> dri/0/crtc-0/crc/control
> dri/0/crtc-0/crc/data
> 
> Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
> start and stop generating frame CRCs and can add entries to the output
> by calling drm_crtc_add_crc_entry.
> 
> v2:
>     - Lots of good fixes suggested by Thierry.
>     - Added documentation.
>     - Changed the debugfs layout.
>     - Moved to allocate the entries circular queue once when frame
>       generation gets enabled for the first time.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> ---
> 
>  Documentation/gpu/drm-uapi.rst    |   6 +
>  drivers/gpu/drm/Makefile          |   3 +-
>  drivers/gpu/drm/drm_crtc.c        |  11 ++
>  drivers/gpu/drm/drm_debugfs.c     |  36 +++-
>  drivers/gpu/drm/drm_debugfs_crc.c | 394 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c         |   9 +
>  drivers/gpu/drm/drm_internal.h    |  10 +
>  include/drm/drmP.h                |   5 +
>  include/drm/drm_crtc.h            |  40 ++++
>  include/drm/drm_debugfs_crc.h     |  71 +++++++
>  10 files changed, 583 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
>  create mode 100644 include/drm/drm_debugfs_crc.h
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 536bf3eaadd4..33f778696ccd 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -109,3 +109,9 @@ interfaces. Especially since all hardware-acceleration interfaces to
>  userspace are driver specific for efficiency and other reasons these
>  interfaces can be rather substantial. Hence every driver has its own
>  chapter.
> +
> +Testing and validation
> +======================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c
> +   :doc: CRC ABI
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e3dba6f44a79..b53b5aaaeb4d 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -12,7 +12,8 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
>  		drm_trace_points.o drm_global.o drm_prime.o \
>  		drm_rect.o drm_vma_manager.o drm_flip_work.o \
> -		drm_modeset_lock.o drm_atomic.o drm_bridge.o
> +		drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> +		drm_debugfs_crc.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 10b73f68c023..48155e0439df 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -40,6 +40,7 @@
>  #include <drm/drm_modeset_lock.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_auth.h>
> +#include <drm/drm_debugfs_crc.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
> @@ -738,6 +739,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	if (cursor)
>  		cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>  
> +#ifdef CONFIG_DEBUG_FS
> +	spin_lock_init(&crtc->crc.lock);
> +	init_waitqueue_head(&crtc->crc.wq);
> +#endif
> +
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&crtc->base, config->prop_active, 0);
>  		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> @@ -764,6 +770,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>  	 * the indices on the drm_crtc after us in the crtc_list.
>  	 */
>  
> +#ifdef CONFIG_DEBUG_FS
> +	drm_debugfs_crtc_remove(crtc);
> +	kfree(crtc->crc.source);
> +#endif
> +
>  	kfree(crtc->gamma_store);
>  	crtc->gamma_store = NULL;
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index fa10cef2ba37..73530cbf1316 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -415,5 +415,39 @@ void drm_debugfs_connector_remove(struct drm_connector *connector)
>  	connector->debugfs_entry = NULL;
>  }
>  
> -#endif /* CONFIG_DEBUG_FS */
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> +	struct drm_minor *minor = crtc->dev->primary;
> +	struct dentry *root;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
> +	if (!name)
> +		return -ENOMEM;
>  
> +	root = debugfs_create_dir(name, minor->debugfs_root);
> +	kfree(name);
> +	if (!root)
> +		return -ENOMEM;
> +
> +	crtc->debugfs_entry = root;
> +
> +	if (drm_debugfs_crtc_crc_add(crtc))
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	debugfs_remove_recursive(crtc->debugfs_entry);
> +	crtc->debugfs_entry = NULL;
> +	return -ENOMEM;
> +}
> +
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> +	debugfs_remove_recursive(crtc->debugfs_entry);
> +
> +	crtc->debugfs_entry = NULL;
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> new file mode 100644
> index 000000000000..7995600bebf0
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -0,0 +1,394 @@
> +/*
> + * Copyright © 2008 Intel Corporation
> + * Copyright © 2016 Collabora Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Eric Anholt <eric@xxxxxxxxxx>
> + *    Keith Packard <keithp@xxxxxxxxxx>

That's neither true for this or the i915 code this is copied from.

> + *
> + */
> +
> +#include <linux/circ_buf.h>
> +#include <linux/ctype.h>
> +#include <linux/debugfs.h>
> +#include <drm/drmP.h>
> +
> +/**
> + * DOC: CRC ABI
> + *
> + * DRM device drivers can provide to userspace CRC information of each frame as
> + * it reached a given hardware component (a "source").
> + *
> + * Userspace can control generation of CRCs in a given CRTC by writing to the
> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
> + * Accepted values are source names (which are driver-specific) and the "none"
> + * and "auto" keywords. "none" will disable CRC generation and "auto" will let
> + * the driver select a default source of frame CRCs for this CRTC.
> + *
> + * Once frame CRC generation is enabled, userspace can capture them by reading
> + * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
> + * number in the first field and then a number of unsigned integer fields
> + * containing the CRC data. Fields are separated by a single space and the number
> + * of CRC fields is source-specific.
> + *
> + * Note that though in some cases the CRC is computed in a specified way and on
> + * the frame contents as supplied by userspace (eDP 1.3), in general the CRC
> + * computation is performed in an unspecified way and on frame contents that have
> + * been already processed in also an unspecified way and thus userspace cannot
> + * rely on being able to generate matching CRC values for the frame contents that
> + * it submits. In this general case, the maximum userspace can do is to compare
> + * the reported CRCs of frames that should have the same contents.
> + */
> +#if defined(CONFIG_DEBUG_FS)

drm-$(CONFIG_DEBUG_FS) ?

And maybe do the same for drm_debugfs.c? 

> +
> +static int crc_control_show(struct seq_file *m, void *data)
> +{
> +	struct drm_crtc *crtc = m->private;
> +
> +	seq_printf(m, "%s\n", crtc->crc.source ? crtc->crc.source : "none");
> +
> +	return 0;
> +}
> +
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> +	struct drm_crtc *crtc = inode->i_private;
> +
> +	return single_open(file, crc_control_show, crtc);
> +}
> +
> +static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
> +{
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +	struct drm_crtc_crc_entry *entries = NULL;
> +	int ret;
> +
> +	if (strcmp(source, "none") == 0)
> +		source = NULL;
> +
> +	if (!crc->source && !source)
> +		return 0;
> +
> +	if (crc->source && source && strcmp(crc->source, source) == 0)
> +		return 0;
> +
> +	if (source) {
> +		if (!crc->entries) {
> +			entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
> +					       sizeof(*crc->entries),
> +					       GFP_KERNEL);
> +			if (!entries)
> +				return -ENOMEM;
> +		}
> +
> +		spin_lock_irq(&crc->lock);
> +		if (crc->entries)
> +			memset(crc->entries, 0,
> +			       sizeof(*crc->entries) * DRM_CRTC_CRC_ENTRIES_NR);
> +		else
> +			crc->entries = entries;
> +		crc->head = 0;
> +		crc->tail = 0;
> +		spin_unlock_irq(&crc->lock);
> +	}
> +
> +	ret = crtc->funcs->set_crc_source(crtc, source);
> +	if (ret)
> +		return ret;
> +
> +	kfree(crc->source);
> +	crc->source = source ? kstrdup(source, GFP_KERNEL) : NULL;
> +
> +	if (!source) {
> +		spin_lock_irq(&crc->lock);
> +		crc->head = 0;
> +		crc->tail = 0;
> +		spin_unlock_irq(&crc->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
> +				 size_t len, loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct drm_crtc *crtc = m->private;
> +	char *source;
> +	int ret;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	if (len > PAGE_SIZE - 1) {
> +		DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
> +			      PAGE_SIZE);
> +		return -E2BIG;
> +	}
> +
> +	source = kmalloc(len + 1, GFP_KERNEL);
> +	if (!source)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(source, ubuf, len)) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (source[len - 1] == '\n')
> +		source[len - 1] = '\0';
> +	else
> +		source[len] = '\0';
> +
> +	ret = crc_control_update_crtc(crtc, source);
> +out:
> +	kfree(source);
> +	if (ret < 0)
> +		return ret;
> +
> +	*offp += len;
> +	return len;
> +}
> +
> +const struct file_operations drm_crtc_crc_control_fops = {
> +	.owner = THIS_MODULE,
> +	.open = crc_control_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +	.write = crc_control_write
> +};
> +
> +static int crtc_crc_open(struct inode *inode, struct file *filep)
> +{
> +	struct drm_crtc *crtc = inode->i_private;
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +
> +	spin_lock_irq(&crc->lock);
> +
> +	if (crc->opened) {
> +		spin_unlock_irq(&crc->lock);
> +		return -EBUSY;
> +	}
> +
> +	crc->opened = true;
> +
> +	spin_unlock_irq(&crc->lock);
> +
> +	return 0;
> +}
> +
> +static int crtc_crc_release(struct inode *inode, struct file *filep)
> +{
> +	struct drm_crtc *crtc = filep->f_inode->i_private;
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +
> +	spin_lock_irq(&crc->lock);
> +	crc->opened = false;
> +	spin_unlock_irq(&crc->lock);
> +
> +	return 0;
> +}

One thing I hate about the i915 code is the fact that it leaves crc
capture on even if you close the file. igt has an exit handler to
try to deal with that problem, but if the kernel just did the right
thing from the start it wouln't be needed. Sadly I failed to convince
Daniel that the kernel shouldn't suck. I still think it's a bad design.

> +
> +/* (6 fields, 8 chars each, space separated (5) + '\n') */
> +#define CRC_LINE_LEN	(6 * 8 + 5 + 1)
> +/* account for \'0' */
> +#define CRC_BUFFER_LEN	(CRC_LINE_LEN + 1)
> +
> +static int crtc_crc_data_count(struct drm_crtc_crc *crc)
> +{
> +	assert_spin_locked(&crc->lock);
> +	return CIRC_CNT(crc->head, crc->tail,
> +			DRM_CRTC_CRC_ENTRIES_NR);
> +}
> +
> +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> +			     size_t count, loff_t *pos)
> +{
> +	struct drm_crtc *crtc = filep->f_inode->i_private;
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +	char buf[CRC_BUFFER_LEN];
> +	int n_entries;
> +	ssize_t bytes_read;
> +
> +	/*
> +	 * Don't allow user space to provide buffers not big enough to hold
> +	 * a line of data.
> +	 */
> +	if (count < CRC_LINE_LEN)
> +		return -EINVAL;
> +
> +	if (!crc->source)
> +		return 0;
> +
> +	/* Nothing to read? */
> +	spin_lock_irq(&crc->lock);
> +	while (crtc_crc_data_count(crc) == 0) {
> +		int ret;
> +
> +		if (filep->f_flags & O_NONBLOCK) {
> +			spin_unlock_irq(&crc->lock);
> +			return -EAGAIN;
> +		}
> +
> +		ret = wait_event_interruptible_lock_irq(crc->wq,
> +				crtc_crc_data_count(crc), crc->lock);
> +		if (ret) {
> +			spin_unlock_irq(&crc->lock);
> +			return ret;
> +		}
> +	}
> +
> +	/* We now have one or more entries to read */
> +	n_entries = count / CRC_LINE_LEN;
> +
> +	bytes_read = 0;
> +	while (n_entries > 0) {
> +		struct drm_crtc_crc_entry *entry =
> +			&crc->entries[crc->tail];
> +		int ret;
> +
> +		if (CIRC_CNT(crc->head, crc->tail, DRM_CRTC_CRC_ENTRIES_NR) < 1)
> +			break;
> +
> +		BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRTC_CRC_ENTRIES_NR);
> +		crc->tail = (crc->tail + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> +
> +		bytes_read += snprintf(buf, CRC_BUFFER_LEN,
> +				       "%8u %8x %8x %8x %8x %8x\n",

You copied the bug from i915. Either make the frame counter hex, or give
it more room to accomodate the full 32 bit number. Long time ago I posted
a patch to use hex in i915, but it got stuck in some review limbo and
as I had more important things to fix, I never revisited the issue.

In i915 we currently stuff the raw hardware frame counter in there, which
isn't all that great since it's only 24 bits on some machines, and doesn't
even exist on others. So userspace can't really deal with wraparound in
any sane way. I guess just switching over to the sw counter universally
could be the most sane way to go. Or maybe we should have room for both?

> +				       entry->frame, entry->crc[0],
> +				       entry->crc[1], entry->crc[2],
> +				       entry->crc[3], entry->crc[4]);
> +
> +		spin_unlock_irq(&crc->lock);
> +
> +		ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
> +		if (ret == CRC_LINE_LEN)
> +			return -EFAULT;
> +
> +		user_buf += CRC_LINE_LEN;
> +		n_entries--;
> +
> +		spin_lock_irq(&crc->lock);
> +	}
> +
> +	spin_unlock_irq(&crc->lock);
> +
> +	return bytes_read;
> +}
> +
> +const struct file_operations drm_crtc_crc_data_fops = {
> +	.owner = THIS_MODULE,
> +	.open = crtc_crc_open,
> +	.read = crtc_crc_read,
> +	.release = crtc_crc_release,
> +};
> +
> +/**
> + * drm_debugfs_crtc_crc_add - Add files to debugfs for capture of frame CRCs
> + * @crtc: CRTC to whom the frames will belong
> + *
> + * Adds files to debugfs directory that allows userspace to control the
> + * generation of frame CRCs and to read them.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +{
> +	struct dentry *crc_ent, *ent;
> +
> +	if (!crtc->funcs->set_crc_source)
> +		return 0;
> +
> +	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> +	if (!crc_ent)
> +		return -ENOMEM;
> +
> +	ent = debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
> +				  &drm_crtc_crc_control_fops);
> +	if (!ent)
> +		goto error;
> +
> +	ent = debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
> +				  &drm_crtc_crc_data_fops);
> +	if (!ent)
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	debugfs_remove_recursive(crc_ent);
> +
> +	return -ENOMEM;
> +}
> +
> +/**
> + * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
> + * @crtc: CRTC to which the frame belongs
> + * @frame: number of the frame characterized by the CRC data
> + * @crc0: piece of data about frame
> + * @crc1: piece of data about frame
> + * @crc2: piece of data about frame
> + * @crc3: piece of data about frame
> + * @crc4: piece of data about frame
> + *
> + * For each frame, the driver polls the source of CRCs for new data and calls
> + * this function to add them to the buffer from where userspace reads.
> + */
> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> +			    uint32_t crc0, uint32_t crc1, uint32_t crc2,
> +			    uint32_t crc3, uint32_t crc4)
> +{
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +	struct drm_crtc_crc_entry *entry;
> +	int head, tail;
> +
> +	spin_lock(&crc->lock);
> +
> +	head = crc->head;
> +	tail = crc->tail;
> +
> +	if (CIRC_SPACE(head, tail, DRM_CRTC_CRC_ENTRIES_NR) < 1) {
> +		spin_unlock(&crc->lock);
> +		DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> +		return;
> +	}
> +
> +	entry = &crc->entries[head];
> +
> +	entry->frame = frame;
> +	entry->crc[0] = crc0;
> +	entry->crc[1] = crc1;
> +	entry->crc[2] = crc2;
> +	entry->crc[3] = crc3;
> +	entry->crc[4] = crc4;
> +
> +	head = (head + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> +	crc->head = head;
> +
> +	spin_unlock(&crc->lock);
> +
> +	wake_up_interruptible(&crc->wq);
> +}
> +EXPORT_SYMBOL_GPL(drm_crtc_add_crc_entry);
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index aead9ffcbe29..a02406f51d98 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -192,6 +192,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
>  static int drm_minor_register(struct drm_device *dev, unsigned int type)
>  {
>  	struct drm_minor *minor;
> +	struct drm_crtc *crtc;
>  	unsigned long flags;
>  	int ret;
>  
> @@ -207,6 +208,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
>  		return ret;
>  	}
>  
> +	if (type == DRM_MINOR_LEGACY) {
> +		drm_for_each_crtc(crtc, dev) {
> +			ret = drm_debugfs_crtc_add(crtc);
> +			if (ret)
> +				goto err_debugfs;
> +		}
> +	}
> +
>  	ret = device_add(minor->kdev);
>  	if (ret)
>  		goto err_debugfs;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b86dc9b921a5..99ce6d4f2916 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -97,6 +97,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  int drm_debugfs_cleanup(struct drm_minor *minor);
>  int drm_debugfs_connector_add(struct drm_connector *connector);
>  void drm_debugfs_connector_remove(struct drm_connector *connector);
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc);
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
>  #else
>  static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  				   struct dentry *root)
> @@ -116,4 +118,12 @@ static inline int drm_debugfs_connector_add(struct drm_connector *connector)
>  static inline void drm_debugfs_connector_remove(struct drm_connector *connector)
>  {
>  }
> +
> +static inline int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> +	return 0;
> +}
> +static inline void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> +}
>  #endif
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index cf918e3e6afb..8a0b235ccc39 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1111,6 +1111,11 @@ static __inline__ bool drm_can_sleep(void)
>  	return true;
>  }
>  
> +#if defined(CONFIG_DEBUG_FS)
> +extern const struct file_operations drm_crc_control_fops;
> +extern const struct file_operations drm_crtc_crc_fops;
> +#endif
> +
>  /* helper for handling conditionals in various for_each macros */
>  #define for_each_if(condition) if (!(condition)) {} else
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 781695c74528..dba8731ef230 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -35,6 +35,7 @@
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> +#include <drm/drm_debugfs_crc.h>
>  
>  struct drm_device;
>  struct drm_mode_set;
> @@ -731,6 +732,29 @@ struct drm_crtc_funcs {
>  	 * before data structures are torndown.
>  	 */
>  	void (*early_unregister)(struct drm_crtc *crtc);
> +
> +	/**
> +	 * @set_crc_source:
> +	 *
> +	 * Changes the source of CRC checksums of frames at the request of
> +	 * userspace, typically for testing purposes. The sources available are
> +	 * specific of each driver and a %NULL value indicates that CRC
> +	 * generation is to be switched off.
> +	 *
> +	 * When CRC generation is enabled, the driver should call
> +	 * drm_crtc_add_crc_entry() at each frame, providing any information
> +	 * that characterizes the frame contents in the crcN arguments, as
> +	 * provided from the configured source. Drivers should accept a "auto"
> +	 * source name that will select a default source for this CRTC.
> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
>  };
>  
>  /**
> @@ -844,6 +868,22 @@ struct drm_crtc {
>  	 * context.
>  	 */
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	/**
> +	 * @debugfs_entry:
> +	 *
> +	 * Debugfs directory for this CRTC.
> +	 */
> +	struct dentry *debugfs_entry;
> +
> +	/**
> +	 * @crc:
> +	 *
> +	 * Configuration settings of CRC capture.
> +	 */
> +	struct drm_crtc_crc crc;
> +#endif
>  };
>  
>  /**
> diff --git a/include/drm/drm_debugfs_crc.h b/include/drm/drm_debugfs_crc.h
> new file mode 100644
> index 000000000000..f526f3f7ac8b
> --- /dev/null
> +++ b/include/drm/drm_debugfs_crc.h
> @@ -0,0 +1,71 @@
> +/*
> + * Copyright © 2016 Collabora Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +#ifndef __DRM_DEBUGFS_CRC_H__
> +#define __DRM_DEBUGFS_CRC_H__
> +
> +/**
> + * struct drm_crtc_crc_entry - entry describing a frame's content
> + * @frame: number of the frame this CRC is about
> + * @crc: array of values that characterize the frame
> + */
> +struct drm_crtc_crc_entry {
> +	uint32_t frame;
> +	uint32_t crc[5];
> +};
> +
> +#define DRM_CRTC_CRC_ENTRIES_NR	128
> +/**
> + * struct drm_crtc_crc - data supporting CRC capture on a given CRTC
> + * @lock: protects the fields in this struct
> + * @source: name of the currently configured source of CRCs
> + * @opened: whether userspace has opened the data file for reading
> + * @entries: array of entries, with size of %DRM_CRTC_CRC_ENTRIES_NR
> + * @head: head of circular queue
> + * @tail: tail of circular queue
> + * @wq: workqueue used to synchronize reading and writing
> + */
> +struct drm_crtc_crc {
> +	spinlock_t lock;
> +	const char *source;
> +	bool opened;
> +	struct drm_crtc_crc_entry *entries;
> +	int head, tail;
> +	wait_queue_head_t wq;
> +};
> +
> +#if defined(CONFIG_DEBUG_FS)
> +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> +			    uint32_t crc0, uint32_t crc1, uint32_t crc2,
> +			    uint32_t crc3, uint32_t crc4);
> +#else
> +static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +{
> +	return 0;
> +}
> +static inline void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> +					  uint32_t crc0, uint32_t crc1,
> +					  uint32_t crc2, uint32_t crc3,
> +					  uint32_t crc4) {}
> +#endif /* defined(CONFIG_DEBUG_FS) */
> +
> +#endif /* __DRM_DEBUGFS_CRC_H__ */
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC