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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> Adds a per-device debugfile "drm_crc_control" that allows selecting a
> source for frame checksums in each CRTC that supports them.
> 
> The checksums for each subsequent frame can be read from the per-CRTC
> file "drm_crtc_N_crc".
> 
> The code is taken from the i915 driver and other drivers can now provide
> frame CRCs by implementing the set_crc_source callback in
> drm_crtc_funcs.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
>  drivers/gpu/drm/drm_crtc.c     |  28 ++-
>  drivers/gpu/drm/drm_debugfs.c  | 506 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_internal.h |  10 +
>  include/drm/drmP.h             |   5 +
>  include/drm/drm_crtc.h         |  72 ++++++
>  5 files changed, 611 insertions(+), 10 deletions(-)

I think we should finalize the internal and external api first, but this
needs a bit better documentation. For that I think it'd be good to extract
this to a new file like drm_debugfs_crc.[hc] and pull that into a new
section (maybe under the testing and validation section, next to the igt
howto) in drm-uapi.rst.

More doc bikeshedding below.

> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e7c862bd2f19..4dae42b122d9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -657,8 +657,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  				       drm_num_crtcs(dev));
>  	}
>  	if (!crtc->name) {
> -		drm_mode_object_unregister(dev, &crtc->base);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_unregister;
>  	}
>  
>  	crtc->base.properties = &crtc->properties;
> @@ -673,12 +673,30 @@ 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);
> +	crtc->crc.debugfs_entries = kmalloc_array(DRM_MINOR_CNT,
> +					  sizeof(*crtc->crc.debugfs_entries),
> +					  GFP_KERNEL);
> +
> +	ret = drm_debugfs_crtc_add(crtc);
> +	if (ret)
> +		goto err_free_name;
> +#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);
>  	}
>  
>  	return 0;
> +
> +err_free_name:
> +	kfree(crtc->name);
> +err_unregister:
> +	drm_mode_object_unregister(dev, &crtc->base);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_crtc_init_with_planes);
>  
> @@ -699,6 +717,12 @@ 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.debugfs_entries);
> +	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..cdc8836bc22a 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -30,6 +30,8 @@
>   * OTHER DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/circ_buf.h>
> +#include <linux/ctype.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
> @@ -127,6 +129,259 @@ fail:
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>  
> +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 crc_control_show(struct seq_file *m, void *data)
> +{
> +	struct drm_device *dev = m->private;
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, dev)
> +		seq_printf(m, "crtc %d %s\n", crtc->index,
> +			   crtc->crc.source ? crtc->crc.source : "none");
> +
> +	return 0;
> +}
> +
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> +	struct drm_device *dev = inode->i_private;
> +
> +	return single_open(file, crc_control_show, dev);
> +}
> +
> +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;
> +	int ret;
> +
> +	if (!strcmp(source, "none"))
> +		source = NULL;
> +
> +	if (!crc->source && !source)
> +		return 0;
> +
> +	if (crc->source && source && !strcmp(crc->source, source))
> +		return 0;
> +
> +	/* Forbid changing the source without going back to "none". */
> +	if (crc->source && source)
> +		return -EINVAL;
> +
> +	if (!crtc->funcs->set_crc_source)
> +		return -ENOTSUPP;
> +
> +	if (source) {
> +		entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
> +				  sizeof(crc->entries[0]),
> +				  GFP_KERNEL);
> +		if (!entries)
> +			return -ENOMEM;
> +
> +		spin_lock_irq(&crc->lock);
> +		kfree(crc->entries);
> +		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);
> +		entries = crc->entries;
> +		crc->entries = NULL;
> +		crc->head = 0;
> +		crc->tail = 0;
> +		spin_unlock_irq(&crc->lock);
> +
> +		kfree(entries);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)
> +{
> +	struct drm_crtc *crtc;
> +	int i = 0;
> +
> +	drm_for_each_crtc(crtc, dev)
> +		if (i++ == index)
> +			return crtc;
> +
> +	return NULL;
> +}
> +
> +/*
> + * Parse CRC command strings:
> + *   command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
> + *   object: ('crtc' | 'pipe')
> + *   crtc: (0 | 1 | 2 | ...)
> + *   pipe: (A | B | C)
> + *   source: (none | plane1 | plane2 | ...)
> + *   wsp: (#0x20 | #0x9 | #0xA)+
> + *
> + * eg.:
> + *  "crtc 0 plane1"  ->  Start CRC computations on plane1 of first CRTC
> + *  "crtc 0 none"    ->  Stop CRC
> + */
> +static int crc_control_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_KMS("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 crc_control_parse_crtc(const char *buf, unsigned int *crtc_index)
> +{
> +	const char letter = buf[0];
> +
> +	if (!kstrtouint(buf, 10, crtc_index))
> +		return 0;
> +
> +	/* Backwards compatibility for Intel-style pipe letters */
> +	if (letter < 'A' || letter > 'Z')
> +		return -EINVAL;
> +
> +	*crtc_index = letter - 'A';
> +
> +	return 0;
> +}
> +
> +static int crc_control_parse(struct drm_device *dev, char *buf, size_t len)
> +{
> +#define N_WORDS 3
> +	int n_words;
> +	char *words[N_WORDS];
> +	unsigned int crtc_index;
> +	struct drm_crtc *crtc;
> +
> +	n_words = crc_control_tokenize(buf, words, N_WORDS);
> +	if (n_words != N_WORDS) {
> +		DRM_DEBUG_KMS("tokenize failed, a command is %d words\n",
> +			      N_WORDS);
> +		return -EINVAL;
> +	}
> +
> +	if (strcmp(words[0], "crtc") && strcmp(words[0], "pipe")) {
> +		DRM_DEBUG_KMS("Invalid command %s\n", words[0]);
> +		return -EINVAL;
> +	}
> +
> +	if (crc_control_parse_crtc(words[1], &crtc_index) < 0) {
> +		DRM_DEBUG_KMS("Invalid CRTC index: %s\n", words[1]);
> +		return -EINVAL;
> +	}
> +
> +	crtc = crtc_from_index(dev, crtc_index);
> +	if (!crtc) {
> +		DRM_DEBUG_KMS("Unknown CRTC index: %d\n", crtc_index);
> +		return -EINVAL;
> +	}
> +
> +	return crc_control_update_crtc(crtc, words[2]);
> +}
> +
> +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_device *dev = m->private;
> +	char *tmpbuf;
> +	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;
> +	}
> +
> +	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 = crc_control_parse(dev, tmpbuf, len);
> +out:
> +	kfree(tmpbuf);
> +	if (ret < 0)
> +		return ret;
> +
> +	*offp += len;
> +	return len;
> +}
> +
> +const struct file_operations drm_crc_control_fops = {
> +	.owner = THIS_MODULE,
> +	.open = crc_control_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +	.write = crc_control_write
> +};
> +
>  /**
>   * Initialize the DRI debugfs filesystem for a device
>   *
> @@ -142,8 +397,9 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root)
>  {
>  	struct drm_device *dev = minor->dev;
> +	struct dentry *ent;
>  	char name[64];
> -	int ret;
> +	int ret = 0;
>  
>  	INIT_LIST_HEAD(&minor->debugfs_list);
>  	mutex_init(&minor->debugfs_lock);
> @@ -157,10 +413,23 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
>  				       minor->debugfs_root, minor);
>  	if (ret) {
> -		debugfs_remove(minor->debugfs_root);
> -		minor->debugfs_root = NULL;
>  		DRM_ERROR("Failed to create core drm debugfs files\n");
> -		return ret;
> +		goto error_remove_dir;
> +	}
> +
> +	ent = debugfs_create_file("drm_crc_control", S_IRUGO | S_IWUSR,
> +				  minor->debugfs_root, dev,
> +				  &drm_crc_control_fops);
> +	if (!ent) {
> +		DRM_ERROR("Failed to create CRC control debugfs files\n");
> +		ret = -ENOMEM;
> +		goto error_remove_files;
> +	}
> +
> +	ret = drm_add_fake_info_node(minor, ent, &drm_crc_control_fops);
> +	if (ret) {
> +		DRM_ERROR("Failed to create CRC control debugfs files\n");
> +		goto error_remove_crc;
>  	}
>  
>  	if (dev->driver->debugfs_init) {
> @@ -171,7 +440,18 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  			return ret;
>  		}
>  	}
> +
>  	return 0;
> +
> +error_remove_crc:
> +	debugfs_remove(ent);
> +error_remove_files:
> +	drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor);
> +error_remove_dir:
> +	debugfs_remove(minor->debugfs_root);
> +	minor->debugfs_root = NULL;
> +
> +	return ret;
>  }
>  
>  
> @@ -407,13 +687,223 @@ error:
>  
>  void drm_debugfs_connector_remove(struct drm_connector *connector)
>  {
> -	if (!connector->debugfs_entry)
> -		return;
> -
>  	debugfs_remove_recursive(connector->debugfs_entry);
>  
>  	connector->debugfs_entry = NULL;
>  }
>  
> -#endif /* CONFIG_DEBUG_FS */
> +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;
> +}
> +
> +/* (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",
> +				       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_fops = {
> +	.owner = THIS_MODULE,
> +	.open = crtc_crc_open,
> +	.read = crtc_crc_read,
> +	.release = crtc_crc_release,
> +};
> +
> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
> +					  struct drm_minor *minor)
> +{
> +	struct dentry *ent;
> +	char *name;
> +
> +	if (!minor->debugfs_root)
> +		return -1;
> +
> +	name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);

I'd check for set_crc_source here and just not add the file if it's not
set. Seems a bit silly to expose an interface when it's not there.
-Daniel

> +	if (!name)
> +		return -ENOMEM;
> +
> +	ent = debugfs_create_file(name, S_IRUGO, minor->debugfs_root, crtc,
> +				  &drm_crtc_crc_fops);
> +	kfree(name);
> +	if (!ent)
> +		return PTR_ERR(ent);
> +
> +	crtc->crc.debugfs_entries[minor->type] = ent;
> +
> +	return 0;
> +}
> +
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> +	int ret;
> +
> +	ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> +	int i;
> +
> +	for (i = 0; i < DRM_MINOR_CNT; i++) {
> +		debugfs_remove_recursive(crtc->crc.debugfs_entries[i]);
> +		crtc->crc.debugfs_entries[i] = NULL;
> +	}
> +}
> +
> +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);
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 38401d406532..e5b124d937f5 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -100,6 +100,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)
> @@ -119,4 +121,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 084fd141e8bf..ec2f91c8b7cd 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1142,6 +1142,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 c2734979f164..141335a3c647 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -376,6 +376,22 @@ struct drm_crtc_state {
>  	struct drm_atomic_state *state;
>  };
>  
> +struct drm_crtc_crc_entry {
> +	uint32_t frame;
> +	uint32_t crc[5];
> +};
> +
> +#define DRM_CRTC_CRC_ENTRIES_NR	128
> +struct drm_crtc_crc {
> +	spinlock_t lock;
> +	const char *source;
> +	bool opened;		/* exclusive access to the result file */
> +	struct drm_crtc_crc_entry *entries;
> +	int head, tail;
> +	wait_queue_head_t wq;
> +	struct dentry **debugfs_entries;
> +};

I think the above two should be moved into drm_debugfs_crc.h and also
documented with kerneldoc.

> +
>  /**
>   * struct drm_crtc_funcs - control CRTCs for a given device
>   *
> @@ -704,6 +720,29 @@ struct drm_crtc_funcs {
>  				   const struct drm_crtc_state *state,
>  				   struct drm_property *property,
>  				   uint64_t *val);
> +
> +	/**
> +	 * @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);
>  };
>  
>  /**
> @@ -817,6 +856,15 @@ struct drm_crtc {
>  	 * context.
>  	 */
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	/**
> +	 * @drm_crtc_crc:
> +	 *
> +	 * Configuration settings of CRC capture.
> +	 */
> +	struct drm_crtc_crc crc;
> +#endif
>  };
>  
>  /**
> @@ -2496,6 +2544,30 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
>  	return 1 << drm_crtc_index(crtc);
>  }
>  
> +/**
> + * 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.
> + */

Generally we put the kerneldoc for functions into the source file. Only
static inlines and structures are documented in the headers.
> +#if defined(CONFIG_DEBUG_FS)
> +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 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
> +
>  extern void drm_connector_ida_init(void);
>  extern void drm_connector_ida_destroy(void);
>  extern int drm_connector_init(struct drm_device *dev,
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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