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 <[email protected]>
> Reviewed-by: Emil Velikov <[email protected]>
> ---
>
> 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
On Tue, Oct 4, 2016 at 12:10 PM, Jon Hunter <[email protected]> wrote:
> 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?
Tegra is still using the load/unload hooks. That didn't mesh well with
Tomeu's patches (and Tomeu's patches have been thrown out meanwhile
because of that). Still would be neat if tegra could be demidlayered
and loose it's load/unload hooks. See the kerneldoc in drm_drv.c
(especially the DOC: section).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Oct 04, 2016 at 01:25:23PM +0200, Daniel Vetter wrote:
> On Tue, Oct 4, 2016 at 12:10 PM, Jon Hunter <[email protected]> wrote:
> > 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?
nouveau is exploding as well on driver load!
> Tegra is still using the load/unload hooks. That didn't mesh well with
> Tomeu's patches (and Tomeu's patches have been thrown out meanwhile
> because of that).
They're still on drm-intel-nightly, please revert if they're broken.
Thanks,
Lukas
On 04/10/16 12:25, Daniel Vetter wrote:
> On Tue, Oct 4, 2016 at 12:10 PM, Jon Hunter <[email protected]> wrote:
>> 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?
>
> Tegra is still using the load/unload hooks. That didn't mesh well with
> Tomeu's patches (and Tomeu's patches have been thrown out meanwhile
> because of that). Still would be neat if tegra could be demidlayered
> and loose it's load/unload hooks. See the kerneldoc in drm_drv.c
> (especially the DOC: section).
Adding Thierry and Alex as this is more their domain and CC'ing linux-tegra.
Cheers
Jon
--
nvpublic
On Tue, Oct 04, 2016 at 02:23:13PM +0200, Lukas Wunner wrote:
> On Tue, Oct 04, 2016 at 01:25:23PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 4, 2016 at 12:10 PM, Jon Hunter <[email protected]> wrote:
> > > 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?
>
> nouveau is exploding as well on driver load!
>
> > Tegra is still using the load/unload hooks. That didn't mesh well with
> > Tomeu's patches (and Tomeu's patches have been thrown out meanwhile
> > because of that).
>
> They're still on drm-intel-nightly, please revert if they're broken.
Never mind, I hadn't rebased on today's drm-intel-nightly yet,
I see it's gone now.
Thanks,
Lukas