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