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

[Thierry Reding]

On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
> +static int
> +drm_add_fake_info_node(struct drm_minor *minor,
> +		       struct dentry *ent,
> +		       const void *key)

Nit: this fits on two lines instead of four.

> +{
> +	struct drm_info_node *node;
> +
> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
> +	if (node == NULL) {
> +		debugfs_remove(ent);

You already remove this in the caller upon returning an error, no need
to do it twice. The caller is where it should be removed.

> +		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;
> +}

Is there a specific reason why you use the drm_info_node infrastructure
here? Seems like it'd be simpler just doing plain debugfs.

> +
> +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;
> +}

Why are these control files not per-CRTC? I'd imagine you could do
something like control the CRC generation on writes and provide the
sampled CRCs on reads.

> +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"))

Nit: I think it's more idiomatic to write the == 0 explicitly for
strcmp() for readability. My brain always interprets !strcmp() as
"strings are not equal", whereas == 0 is more explicit as in "the
difference between strings is 0". But perhaps that's just personal
taste.

> +		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;

Why? It seems to me that if a driver doesn't support switching from one
source to another directly, then it should internally handle that. After
all the source parameter is already driver-specific, so it seems odd to
impose this kind of policy on it at this level.

> +	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);
> +	}

Why are we reallocating? Couldn't we simply allocate once and keep
reusing the existing array?

> +
> +	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);
> +	}

This frees the entries array after resetting source to "none", but what
if we never do that? Aren't we going to leak that data at CRTC removal?

> +static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)

unsigned int index?

> +{
> +	struct drm_crtc *crtc;
> +	int i = 0;

unsigned int?

> +
> +	drm_for_each_crtc(crtc, dev)
> +		if (i++ == index)
> +			return crtc;
> +
> +	return NULL;
> +}

This looks like a candidate for the core. I know that at least Tegra
implements a variant of this, and I think i915 does, too.

> +/*
> + * Parse CRC command strings:
> + *   command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*

Should the "(crtc | pipe)" in the above be "object"?

> + *   object: ('crtc' | 'pipe')

Because you define that here?

> + *   crtc: (0 | 1 | 2 | ...)
> + *   pipe: (A | B | C)
> + *   source: (none | plane1 | plane2 | ...)

I wouldn't provide "plane1 | plane2 |" here, since the parameter is
passed as-is to drivers, which may or may not support plane1 or plane2.

> + *   wsp: (#0x20 | #0x9 | #0xA)+
> + *
> + * eg.:
> + *  "crtc 0 plane1"  ->  Start CRC computations on plane1 of first CRTC
> + *  "crtc 0 none"    ->  Stop CRC

I've said this above, but again, it seems odd to me that you'd have to
configure the CRC per-CRTC in one per-device file and read out the CRC
from per-CRTC files.

> +	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))

Nit: Same comment as for !strcmp().

> +		return 0;
> +
> +	/* Backwards compatibility for Intel-style pipe letters */
> +	if (letter < 'A' || letter > 'Z')
> +		return -EINVAL;
> +
> +	*crtc_index = letter - 'A';
> +
> +	return 0;
> +}

Perhaps it would be more readable to write the above as:

	err = kstrtouint(buf, 10, crtc_index);
	if (err < 0) {
		if (letter < 'A' || letter > 'Z') {
			*crtc_index = letter - 'A';
			err = 0;
		}
	}

	return err;

> +static int crc_control_parse(struct drm_device *dev, char *buf, size_t len)
> +{
> +#define N_WORDS 3
> +	int n_words;

unsigned int?

> +	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",

%u for unsigned int.

> +			      N_WORDS);

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;
> +	}

Propagate the error from crc_control_parse_crtc()?

> +
> +	crtc = crtc_from_index(dev, crtc_index);
> +	if (!crtc) {
> +		DRM_DEBUG_KMS("Unknown CRTC index: %d\n", crtc_index);

%u for unsigned int.

> +		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",

"CRTC" and "CRC", and perhaps a space after <.

> @@ -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;

I think we can do without the error_ prefix on these labels.

> +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;

unsigned int?

> +	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];

Fits on one line.

> +		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",

uint32_t can be 9 characters long in decimal representation. Also I
think it'd be safer to go through a separate variable here, to avoid
inadvertantly subtracting from bytes_read. And then you can also make
the bytes_read variable size_t.

> +				       entry->frame, entry->crc[0],
> +				       entry->crc[1], entry->crc[2],
> +				       entry->crc[3], entry->crc[4]);

What about drivers that only support one uint32_t for the CRC? Do they
also need to output all unused uint32_t:s?

> +
> +		spin_unlock_irq(&crc->lock);

Is it really safe to unlock here? I thought the whole point of this lock
was to prevent someone from reconfiguring the CRC mid-way, but after the
unlock above that's exactly what could happen.

> +
> +		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,
> +};

Do we want to support poll?

> +
> +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;

Can this ever happen? Perhaps turn this into a symbolic name if you
really need it.

> +
> +	name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
> +	if (!name)
> +		return -ENOMEM;

I think it might be preferable to move this all into per-CRTC debugfs
directories, perhaps even collapse the "crc" and "control" files. But in
any case I think the drm_ prefix is redundant here and should be
dropped.

> +	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;
> +}

Do we really want these for all minors? Is ->primary not enough? It
certainly seems completely misplaced in ->render, and I don't think
anything really uses ->control anymore.

Also I think you need to export this symbol.

> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> +	int i;

unsigned int

> +
> +	for (i = 0; i < DRM_MINOR_CNT; i++) {
> +		debugfs_remove_recursive(crtc->crc.debugfs_entries[i]);
> +		crtc->crc.debugfs_entries[i] = NULL;
> +	}
> +}

This also needs an export, I think.

> +
> +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)

Perhaps allow passing the CRC as an array with a count parameter? I can
imagine that a lot of hardware will only give you a single uint32_t for
the CRC, in which case you could do:

	drm_crtc_add_crc_entry(crtc, frame, &crc, 1);

instead of:

	drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);

It would probably save poor users of the interface, such as myself, a
lot of headaches because they can't remember how many uint32_t:s the
function needs.

> +{
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +	struct drm_crtc_crc_entry *entry;
> +	int head, tail;

unsigned int

> +
> +	spin_lock(&crc->lock);
> +
> +	head = crc->head;
> +	tail = crc->tail;
> +
> +	if (CIRC_SPACE(head, tail, DRM_CRTC_CRC_ENTRIES_NR) < 1) {

Perhaps "== 0"?

> +		spin_unlock(&crc->lock);
> +		DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> +		return;
> +	}

Maybe return an error here? And perhaps use some sort of printk rate
limiting here to avoid this from spamming logs?

> +
> +	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);

Oh... this isn't something that drivers are supposed to call?

>  #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 */

You could probably have done this with an atomic and avoid the spin lock
for exclusive access, but it's probably not worth it.

> +	struct drm_crtc_crc_entry *entries;
> +	int head, tail;

unsigned int?

> +	wait_queue_head_t wq;
> +	struct dentry **debugfs_entries;
> +};
> +
>  /**
>   * 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.

Perhaps also mention that "none" is an alias for NULL?

> +	 *
> +	 * 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.

Would it be useful to provide some more aliases? "enable" and "on" for
"auto", "disable" and "off" for "none"?

Thierry

Attachment: signature.asc
Description: PGP signature