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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux