Message-ID: <20160803075721.GH6232@phenom.ffwll.local>
Date:	Wed, 3 Aug 2016 09:57:21 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	Daniel Vetter <daniel.vetter@...el.com>,
	Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs

On Wed, Aug 03, 2016 at 10:06:38AM +0300, Ville Syrjälä wrote:
> On Fri, Jul 22, 2016 at 04:10:44PM +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.
> > v3:
> >     - Use the control file just to select the source, and start and stop
> >       capture when the data file is opened and closed, respectively.
> >     - Make variable the number of CRC values per entry, per source.
> >     - Allocate entries queue each time we start capturing as now there
> >       isn't a fixed number of CRC values per entry.
> >     - Store the frame counter in the data file as a 8-digit hex number.
> >     - For sources that cannot provide useful frame numbers, place
> >       XXXXXXXX in the frame field.
> > 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> > ---
> > 
> >  Documentation/gpu/drm-uapi.rst    |   6 +
> >  drivers/gpu/drm/Makefile          |   3 +-
> >  drivers/gpu/drm/drm_crtc.c        |  12 ++
> >  drivers/gpu/drm/drm_debugfs.c     |  36 +++-
> >  drivers/gpu/drm/drm_debugfs_crc.c | 370 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_drv.c         |   9 +
> >  drivers/gpu/drm/drm_internal.h    |  10 ++
> >  include/drm/drmP.h                |   5 +
> >  include/drm/drm_crtc.h            |  41 +++++
> >  include/drm/drm_debugfs_crc.h     |  74 ++++++++
> >  10 files changed, 564 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..087345af96e7 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,12 @@ 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.source = kstrdup("auto", GFP_KERNEL);
> > +#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 +771,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..5ef071437952
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -0,0 +1,370 @@
> > +/*
> > + * 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@...olt.net>
> > + *    Keith Packard <keithp@...thp.com>
> > + *
> > + */
> > +
> > +#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)
> > +
> > +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);
> > +
> > +	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 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;
> > +	struct drm_crtc_crc *crc = &crtc->crc;
> > +	char *source;
> > +
> > +	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)) {
> > +		kfree(source);
> > +		return -EFAULT;
> > +	}
> 
> memdup_user_nul() ?
> 
> > +
> > +	if (source[len - 1] == '\n')
> > +		source[len - 1] = '\0';
> > +	else
> > +		source[len] = '\0';
> > +
> > +	spin_lock_irq(&crc->lock);
> > +
> > +	if (crc->opened) {
> > +		kfree(source);
> > +		return -EBUSY;
> > +	}
> 
> Why not just start the thing here?
> 
> > +
> > +	kfree(crc->source);
> > +	crc->source = source;
> > +
> > +	spin_unlock_irq(&crc->lock);
> > +
> > +	*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;
> > +	struct drm_crtc_crc_entry *entries = NULL;
> > +	size_t entry_size, values_cnt;
> > +	int ret;
> > +
> > +	if (crc->opened)
> > +		return -EBUSY;
> > +
> > +	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> > +		ret = -EINVAL;
> > +		goto err_disable;
> > +	}
> > +
> > +	if (WARN_ON(values_cnt == 0)) {
> > +		ret = -EINVAL;
> > +		goto err_disable;
> > +	}
> > +
> > +	entry_size = sizeof(*crc->entries) + sizeof(uint32_t) * values_cnt;
> > +	entries = kcalloc(DRM_CRC_ENTRIES_NR, entry_size, GFP_KERNEL);
> > +	if (!entries) {
> > +		ret = -ENOMEM;
> > +		goto err_disable;
> > +	}
> > +
> > +	spin_lock_irq(&crc->lock);
> > +	crc->entries = entries;
> > +	crc->values_cnt = values_cnt;
> > +	crc->opened = true;
> > +	spin_unlock_irq(&crc->lock);
> > +
> > +	return 0;
> > +
> > +err_disable:
> > +	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> > +	return ret;
> > +}
> > +
> > +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;
> > +	size_t values_cnt;
> > +
> > +	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> > +
> > +	spin_lock_irq(&crc->lock);
> > +	kfree(crc->entries);
> > +	crc->entries = NULL;
> > +	crc->head = 0;
> > +	crc->tail = 0;
> > +	crc->values_cnt = 0;
> > +	crc->opened = false;
> > +	spin_unlock_irq(&crc->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int crtc_crc_data_count(struct drm_crtc_crc *crc)
> > +{
> > +	assert_spin_locked(&crc->lock);
> > +	return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
> > +}
> > +
> > +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
> > +						     int index)
> > +{
> > +	void *p = crc->entries;
> > +	size_t entry_size = (sizeof(*crc->entries) +
> > +			     sizeof(*crc->entries[0].crcs) * crc->values_cnt);
> 
> This computation is duplicated also in crtc_crc_open(). could use a
> common helper to do it.
> 
> Shame the language doesn't have a way to deal with arrays of variable
> sized arrays in a nice way.
> 
> > +
> > +	return p + entry_size * index;
> > +}
> > +
> > +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
> > +
> > +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;
> > +	struct drm_crtc_crc_entry *entry;
> > +	char buf[MAX_LINE_LEN];
> > +	int ret, i;
> > +
> > +	spin_lock_irq(&crc->lock);
> > +
> > +	if (!crc->source) {
> > +		spin_unlock_irq(&crc->lock);
> > +		return 0;
> > +	}
> > +
> > +	/* Nothing to read? */
> > +	while (crtc_crc_data_count(crc) == 0) {
> > +		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 know we have an entry to be read */
> > +	entry = crtc_get_crc_entry(crc, crc->tail);
> > +
> > +	/*
> > +	 * 1 frame field of 8 chars plus a number of CRC fields of 8
> > +	 * chars each, space separated and with a newline at the end.
> > +	 */
> > +	if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
> 
> Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
> crc->values_cnt or DRM_MAX_CRC_NR as an argument.
> 
> > +		spin_unlock_irq(&crc->lock);
> > +		return -EINVAL;
> > +	}
> > +
> > +	BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
> > +	crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> > +
> > +	spin_unlock_irq(&crc->lock);

This here is a bit racy, I'd just loop over the ringbuf and emit crcs
one-by-one. That's simpler flow, and we don't care about performance here
at all. But then that's also a bikeshed ;-)

> > +
> > +	if (entry->has_frame_counter)
> > +		snprintf(buf, 9, "%08x", entry->frame);
> > +	else
> > +		snprintf(buf, 9, "XXXXXXXX");
> 
> Should we add "0x" prefix to all these numbers to make it clear that
> they're in fact hex?
> 
> > +
> > +	for (i = 0; i < crc->values_cnt; i++)
> > +		snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
> 
> The 'n' in snprintf() here seems pointless. As does the strlen().
> 
> > +	snprintf(buf + strlen(buf), 2, "\n");
> > +
> > +	if (copy_to_user(user_buf, buf, strlen(buf) + 1))
> > +		return -EFAULT;
> > +
> > +	return strlen(buf) + 1;
> 
> More strlen()s that shouldn't be needed.

Since I just reviewed this on the i915 side: I think we should _not_
include the terminating 0 char in what we copy to userspace and report as
the copied lenght. A file already has out-of-band lenght/eof information,
sprinkling 0 chars at random intervals seems pointless. Random because you
only do it at the end of a read, not after each crc.
-Daniel

> 
> > +}
> > +
> > +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
> > + * @has_frame: whether this entry has a frame number to go with
> > + * @frame: number of the frame these CRCs are about
> > + * @crcs: array of CRC values, with length matching #drm_crtc_crc.values_cnt
> > + *
> > + * 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.
> > + */
> > +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > +			   uint32_t frame, uint32_t *crcs)
> > +{
> > +	struct drm_crtc_crc *crc = &crtc->crc;
> > +	struct drm_crtc_crc_entry *entry;
> > +	int head, tail;
> > +
> > +	assert_spin_locked(&crc->lock);
> > +
> > +	/* Caller may not have noticed yet that userspace has stopped reading */
> > +	if (!crc->opened)
> > +		return -EINVAL;
> > +
> > +	head = crc->head;
> > +	tail = crc->tail;
> > +
> > +	if (CIRC_SPACE(head, tail, DRM_CRC_ENTRIES_NR) < 1) {
> > +		DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> > +		return -ENOBUFS;
> > +	}
> > +
> > +	entry = crtc_get_crc_entry(crc, head);
> > +	entry->frame = frame;
> > +	entry->has_frame_counter = has_frame;
> > +	memcpy(&entry->crcs, crcs, sizeof(*crcs) * crc->values_cnt);
> > +
> > +	head = (head + 1) & (DRM_CRC_ENTRIES_NR - 1);
> > +	crc->head = head;
> > +
> > +	return 0;
> > +}
> > +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..fcc632940dce 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,30 @@ 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,
> > +			      size_t *values_cnt);
> >  };
> >  
> >  /**
> > @@ -844,6 +869,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..a341fc9becad
> > --- /dev/null
> > +++ b/include/drm/drm_debugfs_crc.h
> > @@ -0,0 +1,74 @@
> > +/*
> > + * 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 {
> > +	bool has_frame_counter;
> > +	uint32_t frame;
> > +	uint32_t crcs[0];
> > +};
> > +
> > +#define DRM_MAX_CRC_NR		10
> > +#define DRM_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;
> > +	size_t values_cnt;
> > +	wait_queue_head_t wq;
> > +};
> > +
> > +#if defined(CONFIG_DEBUG_FS)
> > +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> > +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > +			   uint32_t frame, uint32_t *crcs);
> > +#else
> > +static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> > +{
> > +	return 0;
> > +}
> > +static inline int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > +					 uint32_t frame, uint32_t *crcs)
> > +{
> > +	return -EINVAL;
> > +}
> > +#endif /* defined(CONFIG_DEBUG_FS) */
> > +
> > +#endif /* __DRM_DEBUGFS_CRC_H__ */
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@...ts.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Powered by blists - more mailing lists