Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753017AbcJDKLP (ORCPT ); Tue, 4 Oct 2016 06:11:15 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:12309 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751795AbcJDKLN (ORCPT ); Tue, 4 Oct 2016 06:11:13 -0400 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Tue, 04 Oct 2016 03:04:53 -0700 Subject: Re: [PATCH v8 2/4] drm: Add API for capturing frame CRCs To: Tomeu Vizoso , References: <1473415010-1389-1-git-send-email-tomeu.vizoso@collabora.com> <1473415010-1389-3-git-send-email-tomeu.vizoso@collabora.com> CC: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , "Sean Paul" , Daniel Vetter , "Emil Velikov" , Thierry Reding , , David Airlie , , Jonathan Corbet From: Jon Hunter Message-ID: <48ef75ce-061f-d9b0-d5fb-4503af2e5f71@nvidia.com> Date: Tue, 4 Oct 2016 11:10:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <1473415010-1389-3-git-send-email-tomeu.vizoso@collabora.com> X-Originating-IP: [10.21.132.115] X-ClientProxiedBy: DRUKMAIL102.nvidia.com (10.25.59.20) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10759 Lines: 220 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 > Reviewed-by: Emil Velikov > --- > > 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 > #include > #include > -#include > +#include > > #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 : [] lr : [] 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] [] (drm_debugfs_crtc_add) from [] (drm_minor_register+0xa4/0x1b0) [ 7.098405] [] (drm_minor_register) from [] (drm_dev_register+0x7c/0xd0) [ 7.106856] [] (drm_dev_register) from [] (host1x_drm_probe+0x38/0x90) [ 7.115135] [] (host1x_drm_probe) from [] (host1x_device_probe+0x1c/0x28) [ 7.123672] [] (host1x_device_probe) from [] (driver_probe_device+0x1f0/0x2a8) [ 7.132642] [] (driver_probe_device) from [] (bus_for_each_drv+0x44/0x8c) [ 7.141178] [] (bus_for_each_drv) from [] (__device_attach+0x9c/0x100) [ 7.149454] [] (__device_attach) from [] (bus_probe_device+0x84/0x8c) [ 7.157624] [] (bus_probe_device) from [] (device_add+0x380/0x528) [ 7.165551] [] (device_add) from [] (host1x_subdev_register+0xb0/0xd4) [ 7.173827] [] (host1x_subdev_register) from [] (host1x_client_register+0xf4/0x11c) [ 7.183231] [] (host1x_client_register) from [] (tegra_hdmi_probe+0x1c8/0x2c8) [ 7.192201] [] (tegra_hdmi_probe) from [] (platform_drv_probe+0x50/0xb0) [ 7.200653] [] (platform_drv_probe) from [] (driver_probe_device+0x1f0/0x2a8) [ 7.209536] [] (driver_probe_device) from [] (bus_for_each_drv+0x44/0x8c) [ 7.218053] [] (bus_for_each_drv) from [] (__device_attach+0x9c/0x100) [ 7.226326] [] (__device_attach) from [] (bus_probe_device+0x84/0x8c) [ 7.234515] [] (bus_probe_device) from [] (deferred_probe_work_func+0x60/0x8c) [ 7.243486] [] (deferred_probe_work_func) from [] (process_one_work+0x120/0x31c) [ 7.252628] [] (process_one_work) from [] (process_scheduled_works+0x28/0x38) [ 7.261510] [] (process_scheduled_works) from [] (worker_thread+0x204/0x4b4) [ 7.270305] [] (worker_thread) from [] (kthread+0xd8/0xf4) [ 7.277524] [] (kthread) from [] (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