Re: [PATCH v8 2/4] drm: Add API for capturing frame CRCs

From: Jon Hunter
Date: Tue Oct 04 2016 - 06:11:19 EST

Hi Tomeu,

On 09/09/16 10:56, 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.
>
> 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.
>
> v5:
> - Made the crcs array in drm_crtc_crc_entry fixed-size
> - Lots of other smaller improvements suggested by Emil Velikov
>
> v7:
> - Move definition of drm_debugfs_crtc_crc_add to drm_internal.h
>
> v8:
> - Call debugfs_remove_recursive when we fail to create the minor
> device
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> ---
>
> Documentation/gpu/drm-uapi.rst | 6 +
> drivers/gpu/drm/Makefile | 3 +-
> drivers/gpu/drm/drm_crtc.c | 29 +++-
> drivers/gpu/drm/drm_debugfs.c | 34 +++-
> drivers/gpu/drm/drm_debugfs_crc.c | 351 ++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_drv.c | 19 +++
> drivers/gpu/drm/drm_internal.h | 16 ++
> include/drm/drm_crtc.h | 41 +++++
> include/drm/drm_debugfs_crc.h | 73 ++++++++
> 9 files changed, 569 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
> create mode 100644 include/drm/drm_debugfs_crc.h

...

> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -40,7 +40,7 @@
> #include <drm/drm_modeset_lock.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_auth.h>
> -#include <drm/drm_framebuffer.h>
> +#include <drm/drm_debugfs_crc.h>
>
> #include "drm_crtc_internal.h"
> #include "drm_internal.h"
> @@ -141,6 +141,25 @@ static void drm_crtc_unregister_all(struct drm_device *dev)
> }
> }
>
> +static int drm_crtc_crc_init(struct drm_crtc *crtc)
> +{
> +#ifdef CONFIG_DEBUG_FS
> + spin_lock_init(&crtc->crc.lock);
> + init_waitqueue_head(&crtc->crc.wq);
> + crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> + if (!crtc->crc.source)
> + return -ENOMEM;
> +#endif
> + return 0;
> +}
> +
> +static void drm_crtc_crc_fini(struct drm_crtc *crtc)
> +{
> +#ifdef CONFIG_DEBUG_FS
> + kfree(crtc->crc.source);
> +#endif
> +}
> +
> /**
> * drm_crtc_init_with_planes - Initialise a new CRTC object with
> * specified primary and cursor planes.
> @@ -206,6 +225,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);
>
> + ret = drm_crtc_crc_init(crtc);
> + if (ret) {
> + drm_mode_object_unregister(dev, &crtc->base);
> + return ret;
> + }
> +
> 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);
> @@ -232,6 +257,8 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
> * the indices on the drm_crtc after us in the crtc_list.
> */
>
> + drm_crtc_crc_fini(crtc);
> +
> 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 1205790ed960..800055c39cdb 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -415,5 +415,37 @@ 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;

After this patch was applied Tegra boards started crashing here when
dereferencing crtc pointer above ...

[ 6.789318] Unable to handle kernel paging request at virtual address fffffff8
[ 6.796537] pgd = c0004000
[ 6.799270] [fffffff8] *pgd=afffd861, *pte=00000000, *ppte=00000000
[ 6.805572] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
[ 6.810969] Modules linked in:
[ 6.814038] CPU: 2 PID: 72 Comm: kworker/2:1 Not tainted 4.8.0-next-20161004-126151-gc7d3b91 #1
[ 6.822728] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 6.829000] Workqueue: events deferred_probe_work_func
[ 6.834150] task: ee00f880 task.stack: ee010000
[ 6.838682] PC is at drm_debugfs_crtc_add+0x8/0x70
[ 6.843481] LR is at drm_minor_register+0xa4/0x1b0
[ 6.848267] pc : [<c0440b80>] lr : [<c0428a60>] psr: a0000113
[ 6.848267] sp : ee011d20 ip : ee2f4914 fp : c0d02100
[ 6.859732] r10: 00000001 r9 : 00000000 r8 : 00000000
[ 6.864949] r7 : ee2f3800 r6 : ee2f3a4c r5 : ee2f4900 r4 : fffffff8
[ 6.871472] r3 : 00000001 r2 : 00000000 r1 : ee011c70 r0 : fffffff8
[ 6.877992] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 6.885123] Control: 10c5387d Table: 8000406a DAC: 00000051
[ 6.890871] Process kworker/2:1 (pid: 72, stack limit = 0xee010210)
[ 6.897129] Stack: (0xee011d20 to 0xee012000)
[ 6.901491] 1d20: fffffff8 ee2f4900 ee2f3a4c c0428a60 ee00f880 ee2f3800 00000000 00000000
[ 6.909670] 1d40: c0d34794 0000001e 00000000 c0428be8 ee2f3800 eebae800 c0d3467c c0442008
[ 6.917839] 1d60: eebae818 00000000 c0d34794 0000001e eebae810 c0dabfec 00000000 c0407578
[ 6.926014] 1d80: c040755c c045ac3c 00000000 ee011db8 c045af10 00000001 c0dabfc8 c045922c
[ 6.934190] 1da0: eeaaa370 eebac0b8 eebae810 eebae844 c0d33fa0 c045a9c4 eebae810 00000001
[ 6.942366] 1dc0: c0d02100 eebae810 eebae810 c0d33fa0 ee9b6e10 c045a0b4 eebae810 00000000
[ 6.950544] 1de0: eebae818 c0458624 c0d6404c 60000113 c0d02100 c0817664 eebae800 ee2f3c10
[ 6.958713] 1e00: ee034ec0 eebae9d8 eebae9b0 eefa5580 eebae810 c0407744 eefbf974 eebaeb94
[ 6.966887] 1e20: c0d3400c c0d33f68 ee2f3c10 eebaea10 00000000 c0408058 ee2f3c10 ee9b6810
[ 6.975064] 1e40: 00000000 ee9b6800 00000000 c044bb4c 00000000 ee9b4940 ee2f3c10 00000000
[ 6.983241] 1e60: ffffffed ee9b6810 fffffdfb c0d348f8 0000001e c045c20c c045c1bc ee9b6810
[ 6.991417] 1e80: c0dabfec 00000000 c0d348f8 c045ac3c 00000000 ee011ec0 c045af10 00000001
[ 6.999595] 1ea0: 00000000 c045922c ee8a3d70 eebacfb8 ee9b6810 ee9b6844 c0d34de8 c045a9c4
[ 7.007763] 1ec0: ee9b6810 00000001 c0d02100 ee9b6810 ee9b6810 c0d34de8 eefa8800 c045a0b4
[ 7.015938] 1ee0: ee9b6810 c0d34d70 c0d34d90 c045a4e8 eeb78f00 c0d34d98 eefa5580 c0134890
[ 7.024115] 1f00: ee171484 eefa5580 eeb78f00 eeb78f18 00000001 eefa5580 eeb78f18 eefa5580
[ 7.032292] 1f20: 00000008 c0134ab4 eefa88f5 eeb78f00 eefa5598 c0134cc8 00000000 eeaf6fc0
[ 7.040469] 1f40: 00000000 eeaf6fc0 00000000 eeb78f00 c0134ac4 00000000 00000000 00000000
[ 7.048637] 1f60: 00000000 c0139d68 fffcffff 00000000 ffefffff eeb78f00 00000000 00000000
[ 7.056812] 1f80: ee011f80 ee011f80 00000000 00000000 ee011f90 ee011f90 ee011fac eeaf6fc0
[ 7.064988] 1fa0: c0139c90 00000000 00000000 c0107938 00000000 00000000 00000000 00000000
[ 7.073165] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 7.081342] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 fffffff3 ffffefbf
[ 7.089538] [<c0440b80>] (drm_debugfs_crtc_add) from [<c0428a60>] (drm_minor_register+0xa4/0x1b0)
[ 7.098405] [<c0428a60>] (drm_minor_register) from [<c0428be8>] (drm_dev_register+0x7c/0xd0)
[ 7.106856] [<c0428be8>] (drm_dev_register) from [<c0442008>] (host1x_drm_probe+0x38/0x90)
[ 7.115135] [<c0442008>] (host1x_drm_probe) from [<c0407578>] (host1x_device_probe+0x1c/0x28)
[ 7.123672] [<c0407578>] (host1x_device_probe) from [<c045ac3c>] (driver_probe_device+0x1f0/0x2a8)
[ 7.132642] [<c045ac3c>] (driver_probe_device) from [<c045922c>] (bus_for_each_drv+0x44/0x8c)
[ 7.141178] [<c045922c>] (bus_for_each_drv) from [<c045a9c4>] (__device_attach+0x9c/0x100)
[ 7.149454] [<c045a9c4>] (__device_attach) from [<c045a0b4>] (bus_probe_device+0x84/0x8c)
[ 7.157624] [<c045a0b4>] (bus_probe_device) from [<c0458624>] (device_add+0x380/0x528)
[ 7.165551] [<c0458624>] (device_add) from [<c0407744>] (host1x_subdev_register+0xb0/0xd4)
[ 7.173827] [<c0407744>] (host1x_subdev_register) from [<c0408058>] (host1x_client_register+0xf4/0x11c)
[ 7.183231] [<c0408058>] (host1x_client_register) from [<c044bb4c>] (tegra_hdmi_probe+0x1c8/0x2c8)
[ 7.192201] [<c044bb4c>] (tegra_hdmi_probe) from [<c045c20c>] (platform_drv_probe+0x50/0xb0)
[ 7.200653] [<c045c20c>] (platform_drv_probe) from [<c045ac3c>] (driver_probe_device+0x1f0/0x2a8)
[ 7.209536] [<c045ac3c>] (driver_probe_device) from [<c045922c>] (bus_for_each_drv+0x44/0x8c)
[ 7.218053] [<c045922c>] (bus_for_each_drv) from [<c045a9c4>] (__device_attach+0x9c/0x100)
[ 7.226326] [<c045a9c4>] (__device_attach) from [<c045a0b4>] (bus_probe_device+0x84/0x8c)
[ 7.234515] [<c045a0b4>] (bus_probe_device) from [<c045a4e8>] (deferred_probe_work_func+0x60/0x8c)
[ 7.243486] [<c045a4e8>] (deferred_probe_work_func) from [<c0134890>] (process_one_work+0x120/0x31c)
[ 7.252628] [<c0134890>] (process_one_work) from [<c0134ab4>] (process_scheduled_works+0x28/0x38)
[ 7.261510] [<c0134ab4>] (process_scheduled_works) from [<c0134cc8>] (worker_thread+0x204/0x4b4)
[ 7.270305] [<c0134cc8>] (worker_thread) from [<c0139d68>] (kthread+0xd8/0xf4)
[ 7.277524] [<c0139d68>] (kthread) from [<c0107938>] (ret_from_fork+0x14/0x3c)
[ 7.284749] Code: e584334c e8bd8010 e92d4070 e1a04000 (e5943000)
[ 7.290952] ---[ end trace 988a54919c1729f9 ]---

Looks like crtc is a errno in the above case. I see this function is
called by looping through all the crtc and we never check to see if
they are valid. Should we?

Cheers
Jon

--
nvpublic