On 6 October 2016 at 12:33, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote: > On 10/06/2016 01:13 PM, Emil Velikov wrote: >> On 6 October 2016 at 09:56, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> 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 >>> >>> v9: >>> - Register the debugfs directory for a crtc from >>> drm_crtc_register_all() >>> >>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> >>> --- >>> >>> Documentation/gpu/drm-uapi.rst | 6 + >>> drivers/gpu/drm/Makefile | 3 +- >>> drivers/gpu/drm/drm_crtc.c | 34 +++- >>> drivers/gpu/drm/drm_debugfs.c | 34 +++- >>> drivers/gpu/drm/drm_debugfs_crc.c | 351 ++++++++++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/drm_internal.h | 16 ++ >>> include/drm/drm_crtc.h | 41 +++++ >>> include/drm/drm_debugfs_crc.h | 73 ++++++++ >>> 8 files changed, 555 insertions(+), 3 deletions(-) >>> create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c >>> create mode 100644 include/drm/drm_debugfs_crc.h >>> >>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst >>> index 1ba301cebe16..de3ac9f90f8f 100644 >>> --- a/Documentation/gpu/drm-uapi.rst >>> +++ b/Documentation/gpu/drm-uapi.rst >>> @@ -216,3 +216,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 25c720454017..74579d2e796e 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 \ >>> @@ -23,6 +23,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 2d7bedf28647..151ff9805de1 100644 >>> --- 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" >>> @@ -126,6 +126,10 @@ static int drm_crtc_register_all(struct drm_device *dev) >>> ret = crtc->funcs->late_register(crtc); >>> if (ret) >> Here we want to teardown the already created debugfs entries. > > Yeah, I was a bit puzzled by the existing behaviour of > drm_modeset_register_all, as if we fail when trying to register the > second crtc, we'll unregister all planes but leave the first crtc there > (and so on with the other objects). > > So I'm not really sure on whether it would be good or bad to remove the > debugfs entries of the CRTCs that did register when another CRTC fails > to register. > Ack. Seems like only the drm_connector_register_all does proper cleanup, on failure, and plane, crtc and encoder do not. Since things are already in a funny shape it might be worth getting this as-is and fixing the lot as a follow up ? Mildly related: seems like we create the connector sysfs/debugfs entries prior to the actual HW setup (driver callback). Worth keeping things symmetrical across crtc/connector ? Thanks, Emil _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
