diff mbox

[v4,2/4] drm: Add API for capturing frame CRCs

Message ID 1470393905-6640-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomeu Vizoso Aug. 5, 2016, 10:45 a.m. UTC
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.

v4:
    - Build only if CONFIG_DEBUG_FS is enabled.
    - Use memdup_user_nul.
    - Consolidate calculation of the size of an entry in a helper.
    - Add 0x prefix to hex numbers in the data file.
    - Remove unnecessary snprintf and strlen usage in read callback.

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 | 368 ++++++++++++++++++++++++++++++++++++++
 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, 562 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
 create mode 100644 include/drm/drm_debugfs_crc.h

Comments

Emil Velikov Sept. 2, 2016, 2:50 p.m. UTC | #1
Hi Tomeu,

On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:

> +#ifdef CONFIG_DEBUG_FS
> +       spin_lock_init(&crtc->crc.lock);
> +       init_waitqueue_head(&crtc->crc.wq);
> +       crtc->crc.source = kstrdup("auto", GFP_KERNEL);
Pedantic: kstrdup() can never fail ?

> +#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
> +
Worth adding these two as static inlines in the header ?

Things are a bit asymetrical here. drm_debugfs_crtc_add() is in
drm_minor_register(), so why isn't drm_debugfs_crtc_remove() in
drm_minor_free() ?



> -#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);
Mildly related: some userspace projects define convenience helpers
which you use in order to allocate the correct amount of space on
stack.
Is there such a set for the kernel ?

With those the above will become something like:
char name[5+DECIMAL_STR_MAX] = ksprintf("crtc-%d", crtc->index);



> --- /dev/null
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c

> + *
> + * Authors:
> + *    Eric Anholt <eric@anholt.net>
> + *    Keith Packard <keithp@keithp.com>
> + *
Wondering about these authorship lines - is the code copied/derived
from somewhere ? If so please mention where from.

> +/*
> + * 1 frame field of 10 chars plus a number of CRC fields of 10 chars each, space
> + * separated and with a newline at the end.
> + */
> +#define LINE_LEN(VALUES_CNT)   (10 + 11 * VALUES_CNT + 1 + 1)
Hmm I seem to suck at math. The macro seems larger than expected (comment and
code wise) by 1, right ?

> +#define MAX_LINE_LEN           (LINE_LEN(DRM_MAX_CRC_NR))
> +
Worth using lowercase VALUES_CNT and less likely to clash macro names ?



> +       BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
> +       crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
It's strange that the kernel does not have a macro for this. But for the sake
of me I cannot find one.



> --- 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) {
Sidenote: If we did not use DRM_MINOR_LEGACY for render-only GPUs we would
save a couple of cycles here :-)

> +               drm_for_each_crtc(crtc, dev) {
> +                       ret = drm_debugfs_crtc_add(crtc);
> +                       if (ret)
> +                               goto err_debugfs;
IMHO we don't want to bring down the whole setup if this fails, unlike
where we cannot create debug/dri all together.

Regardless: we seems to be missing the cleanup in the error path ?



> +#if defined(CONFIG_DEBUG_FS)
> +extern const struct file_operations drm_crc_control_fops;
> +extern const struct file_operations drm_crtc_crc_fops;
These seems to be named differently in the code. Namely:

drm_crtc_crc_control_fops
drm_crtc_crc_data_fops

Then again, you don't seem to use the outside of the file, so I'd make them
static and drop this hunk.



>          * 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"
Very nicely written documentation. Just to make it more obvious - s/should/must/


> +/**
> + * 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;
Missing documentation. What is/should frame (below) going to contain if false ?

> +       uint32_t frame;
> +       uint32_t crcs[0];
Likely the only one here: using a zero sized array is a GNU extension which we
can avoid. Esp since we cap the size to 10 (DRM_MAX_CRC_NR).

> +};
> +
> +#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;
Missing documentation. Please explicitly state its relation to DRM_MAX_CRC_NR,
as you did with entries and DRM_CRTC_CRC_ENTRIES_NR. Worth moving where it's
applicable/used - i.e. in drm_crtc_crc_entry ?


Regards,
Emil
Tomeu Vizoso Sept. 5, 2016, 9:02 a.m. UTC | #2
On 2 September 2016 at 16:50, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Tomeu,
>
> On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
>> +#ifdef CONFIG_DEBUG_FS
>> +       spin_lock_init(&crtc->crc.lock);
>> +       init_waitqueue_head(&crtc->crc.wq);
>> +       crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> Pedantic: kstrdup() can never fail ?

Dealt with.

>> +#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
>> +
> Worth adding these two as static inlines in the header ?

Have moved the ifdefs to static functions in the same file.

> Things are a bit asymetrical here. drm_debugfs_crtc_add() is in
> drm_minor_register(), so why isn't drm_debugfs_crtc_remove() in
> drm_minor_free() ?

Good call. Have moved drm_debugfs_crtc_remove to drm_minor_unregister.

>> -#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);
> Mildly related: some userspace projects define convenience helpers
> which you use in order to allocate the correct amount of space on
> stack.
> Is there such a set for the kernel ?
>
> With those the above will become something like:
> char name[5+DECIMAL_STR_MAX] = ksprintf("crtc-%d", crtc->index);

Don't know of those but I think that in this specific case an
allocation is not that much of a problem.

>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>
>> + *
>> + * Authors:
>> + *    Eric Anholt <eric@anholt.net>
>> + *    Keith Packard <keithp@keithp.com>
>> + *
> Wondering about these authorship lines - is the code copied/derived
> from somewhere ? If so please mention where from.

Done, and also fixed the actual people that should be in there.

>> +/*
>> + * 1 frame field of 10 chars plus a number of CRC fields of 10 chars each, space
>> + * separated and with a newline at the end.
>> + */
>> +#define LINE_LEN(VALUES_CNT)   (10 + 11 * VALUES_CNT + 1 + 1)
> Hmm I seem to suck at math. The macro seems larger than expected (comment and
> code wise) by 1, right ?

The comment indeed should mention that it's NULL-terminated, but the
code is correct, isn't it?

>> +#define MAX_LINE_LEN           (LINE_LEN(DRM_MAX_CRC_NR))
>> +
> Worth using lowercase VALUES_CNT and less likely to clash macro names ?

Done.

>
>
>> +       BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
>> +       crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> It's strange that the kernel does not have a macro for this. But for the sake
> of me I cannot find one.

Yeah.

>> --- 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) {
> Sidenote: If we did not use DRM_MINOR_LEGACY for render-only GPUs we would
> save a couple of cycles here :-)
>
>> +               drm_for_each_crtc(crtc, dev) {
>> +                       ret = drm_debugfs_crtc_add(crtc);
>> +                       if (ret)
>> +                               goto err_debugfs;
> IMHO we don't want to bring down the whole setup if this fails, unlike
> where we cannot create debug/dri all together.
>
> Regardless: we seems to be missing the cleanup in the error path ?

Have changed it to only log an error and continue, so no cleanup is necessary.

>> +#if defined(CONFIG_DEBUG_FS)
>> +extern const struct file_operations drm_crc_control_fops;
>> +extern const struct file_operations drm_crtc_crc_fops;
> These seems to be named differently in the code. Namely:
>
> drm_crtc_crc_control_fops
> drm_crtc_crc_data_fops
>
> Then again, you don't seem to use the outside of the file, so I'd make them
> static and drop this hunk.

Oops.

>
>
>>          * 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"
> Very nicely written documentation. Just to make it more obvious - s/should/must/

Ok.

>> +/**
>> + * 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;
> Missing documentation. What is/should frame (below) going to contain if false ?

Ok.

>> +       uint32_t frame;
>> +       uint32_t crcs[0];
> Likely the only one here: using a zero sized array is a GNU extension which we
> can avoid. Esp since we cap the size to 10 (DRM_MAX_CRC_NR).

Agreed, I also think that the added code complexity isn't worth the
reduced memory usage.

>> +};
>> +
>> +#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;
> Missing documentation. Please explicitly state its relation to DRM_MAX_CRC_NR,
> as you did with entries and DRM_CRTC_CRC_ENTRIES_NR. Worth moving where it's
> applicable/used - i.e. in drm_crtc_crc_entry ?

Done.

Lots of good stuff, thanks a lot!

Tomeu
diff mbox

Patch

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..a920b4d60853 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -9,7 +9,7 @@  drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_scatter.o drm_pci.o \
 		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
 		drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o \
-		drm_info.o drm_debugfs.o drm_encoder_slave.o \
+		drm_info.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
@@ -20,6 +20,7 @@  drm-$(CONFIG_PCI) += ati_pcigart.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
+drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f1d9f0569d7f..59f2b2746b91 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..b24d6fb924ef
--- /dev/null
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -0,0 +1,368 @@ 
+/*
+ * 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 "auto"
+ * keyword, which 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.
+ */
+
+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 = memdup_user_nul(ubuf, len);
+	if (IS_ERR(source))
+		return PTR_ERR(source);
+
+	if (source[len] == '\n')
+		source[len] = '\0';
+
+	spin_lock_irq(&crc->lock);
+
+	if (crc->opened) {
+		spin_unlock_irq(&crc->lock);
+		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 size_t crtc_get_crc_entry_size(struct drm_crtc_crc *crc,
+				      size_t values_cnt)
+{
+	return sizeof(*crc->entries) + sizeof(*crc->entries[0].crcs) *
+	       values_cnt;
+}
+
+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 = crtc_get_crc_entry_size(crc, 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;
+
+	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);
+
+	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+
+	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;
+
+	return p + crtc_get_crc_entry_size(crc, crc->values_cnt) * index;
+}
+
+/*
+ * 1 frame field of 10 chars plus a number of CRC fields of 10 chars each, space
+ * separated and with a newline at the end.
+ */
+#define LINE_LEN(VALUES_CNT)	(10 + 11 * VALUES_CNT + 1 + 1)
+#define MAX_LINE_LEN		(LINE_LEN(DRM_MAX_CRC_NR))
+
+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);
+
+	if (count < LINE_LEN(crc->values_cnt)) {
+		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)
+		sprintf(buf, "0x%08x", entry->frame);
+	else
+		sprintf(buf, "XXXXXXXXXX");
+
+	for (i = 0; i < crc->values_cnt; i++)
+		sprintf(buf + 10 + i * 11, " 0x%08x", entry->crcs[i]);
+	sprintf(buf + 10 + crc->values_cnt * 11, "\n");
+
+	if (copy_to_user(user_buf, buf, LINE_LEN(crc->values_cnt)))
+		return -EFAULT;
+
+	return LINE_LEN(crc->values_cnt);
+}
+
+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);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index be27ed36f56e..80ae9edac8bd 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 d3778652e462..9d88a9ba8a58 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1032,6 +1032,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 3edeaf88ebc0..49237b123dcb 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);
 };
 
 /**
@@ -847,6 +872,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__ */