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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160621/b8bdf25e/attachment-0001.sig>
