Message-ID: <20160621150758.GB21079@ulmo.ba.sec>
Date:	Tue, 21 Jun 2016 17:07:58 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	linux-kernel@...r.kernel.org,
	Daniel Vetter <daniel.vetter@...el.com>,
	dri-devel@...ts.freedesktop.org,
	Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs

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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists