| Message ID | 1469196645-3061-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@collabora.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@anholt.net> > + * Keith Packard <keithp@keithp.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); > + > + 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. > +} > + > +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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@collabora.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@anholt.net> > > + * Keith Packard <keithp@keithp.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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 3 August 2016 at 09:06, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@collabora.com> >> --- ... >> +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() ? Good call. >> + >> + 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? For the sake of symmetry, as we are stopping when the data file is closed. >> +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. Ok. >> + >> + 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. Sounds good, went with a macro. >> + 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); >> + >> + 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? Sounds like a good idea to me. >> + >> + 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(). Good. >> + 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. Ok. Thanks! Tomeu
On Fri, Aug 05, 2016 at 12:46:29PM +0200, Tomeu Vizoso wrote: > On 3 August 2016 at 09:06, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@collabora.com> > >> --- > ... > >> +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() ? > > Good call. > > >> + > >> + 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? > > For the sake of symmetry, as we are stopping when the data file is closed. Yes, but if the data file is already open, we should start as soon as the source is configured. Or are you redusing to open the data file w/o a source selected? > > >> +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. > > Ok. > > >> + > >> + 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. > > Sounds good, went with a macro. > > >> + 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); > >> + > >> + 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? > > Sounds like a good idea to me. > > >> + > >> + 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(). > > Good. > > >> + 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. > > Ok. > > Thanks! > > Tomeu
Hi Tomeu, On 22 July 2016 at 15:10, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > +/** > + * 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 s/can/must/ Is it worth having 'auto' as a default source perhaps? > + * 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. Is it also worth having 'connector-%s' (named as per sysfs, e.g. connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which have CRC control on the connector rather than the CRTC? Cheers, Daniel
On 6 August 2016 at 19:04, Daniel Stone <daniel@fooishbar.org> wrote: > Hi Tomeu, > > On 22 July 2016 at 15:10, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> +/** >> + * 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 > > s/can/must/ > > Is it worth having 'auto' as a default source perhaps? Yup, it's the case in v4, so you just cat the data file and start getting CRCs. >> + * 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. > > Is it also worth having 'connector-%s' (named as per sysfs, e.g. > connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which > have CRC control on the connector rather than the CRTC? My impression right now is that only "auto" makes sense as a standardised entry, as any explicit sources are pretty much hw-dependent so the tests will need knowledge about the hw anyway. The IGT tests already try each connector in each CRTC when looking for a setup that supports CRC capture (with the auto source). Regards, Tomeu
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@anholt.net> + * Keith Packard <keithp@keithp.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; + } + + 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; + } + + 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); + + 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) { + 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); + + if (entry->has_frame_counter) + snprintf(buf, 9, "%08x", entry->frame); + else + snprintf(buf, 9, "XXXXXXXX"); + + for (i = 0; i < crc->values_cnt; i++) + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]); + snprintf(buf + strlen(buf), 2, "\n"); + + if (copy_to_user(user_buf, buf, strlen(buf) + 1)) + return -EFAULT; + + return strlen(buf) + 1; +} + +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__ */
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@collabora.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