[PATCH v3 2/3] drm: Add API for capturing frame CRCs
On 6 August 2016 at 19:04, Daniel Stone wrote: > Hi Tomeu, > > On 22 July 2016 at 15:10, Tomeu Vizoso 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
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
Hi Tomeu, On 22 July 2016 at 15:10, Tomeu Vizoso 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
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
On Fri, Aug 05, 2016 at 12:46:29PM +0200, Tomeu Vizoso wrote:
> On 3 August 2016 at 09:06, 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
> >> in the frame field.
> >>
> >> Signed-off-by: Tomeu Vizoso
> >> ---
> ...
> >> +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 = >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(>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 = >crc;
> >> + struct drm_crtc_crc_entry *entry;
> >> + char buf[MAX_LINE_LEN];
> >> + int ret, i;
> >> +
> >> + spin_lock_irq(>lock);
> >> +
> >> + if (!crc->source) {
> >> + spin_unlock_irq(>lock);
> >> + return 0;
> >> + }
> >> +
> >> + /* Nothing to read? */
> >> + while (crtc_crc_data_count(crc) == 0) {
> >> + if (filep->f_flags & O_NONBLOCK) {
> >> + spin_unlock_irq(>lock);
> >> + return -EAGAIN;
> >> + }
> >> +
> >> + ret = wait_event_interruptible_lock_irq(crc->wq,
> >> +
> >> crtc_crc_data_count(crc),
> >> + crc->lock);
> >> + if (ret) {
> >> + spin_unlock_irq(>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
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
On 3 August 2016 at 09:06, 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
>> in the frame field.
>>
>> Signed-off-by: Tomeu Vizoso
>> ---
...
>> +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 = >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(>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 = >crc;
>> + struct drm_crtc_crc_entry *entry;
>> + char buf[MAX_LINE_LEN];
>> + int ret, i;
>> +
>> + spin_lock_irq(>lock);
>> +
>> + if (!crc->source) {
>> + spin_unlock_irq(>lock);
>> + return 0;
>> + }
>> +
>> + /* Nothing to read? */
>> + while (crtc_crc_data_count(crc) == 0) {
>> + if (filep->f_flags & O_NONBLOCK) {
>> + spin_unlock_irq(>lock);
>> + return -EAGAIN;
>> + }
>> +
>> + ret = wait_event_interruptible_lock_irq(crc->wq,
>> +
>> crtc_crc_data_count(crc),
>> + crc->lock);
>> + if (ret) {
>> + spin_unlock_irq(>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(>lock);
>> + return -EINVAL;
>> + }
>> +
>> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
>> + crc->tail = (crc->tail + 1) &
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
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
> in the frame field.
>
> Signed-off-by: Tomeu Vizoso
> ---
>
> 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
> #include
> #include
> +#include
>
> #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(>crc.lock);
> + init_waitqueue_head(>crc.wq);
> + crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> +#endif
> +
> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> drm_object_attach_property(>base, config->prop_active, 0);
> drm_object_attach_property(>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",
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
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
> > in the frame field.
> >
> > Signed-off-by: Tomeu Vizoso
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > #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(>crc.lock);
> > + init_waitqueue_head(>crc.wq);
> > + crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> > +#endif
> > +
> > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > drm_object_attach_property(>base, config->prop_active, 0);
> > drm_object_attach_property(>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)
> >
[PATCH v3 2/3] drm: Add API for capturing frame CRCs
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
in the frame field.
Signed-off-by: Tomeu Vizoso
---
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
#include
#include
+#include
#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(>crc.lock);
+ init_waitqueue_head(>crc.wq);
+ crtc->crc.source = kstrdup("auto", GFP_KERNEL);
+#endif
+
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
drm_object_attach_property(>base, config->prop_active, 0);
drm_object_attach_property(>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
