On 10/06/2016 01:13 PM, Emil Velikov wrote:
On 6 October 2016 at 09:56, Tomeu Vizoso tomeu.vizoso@collabora.com 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@collabora.com
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.
return ret;
ret = drm_debugfs_crtc_add(crtc);if (ret)return ret;IIRC v8 made sure we don't error out if adding debugfs entries fails. Pretty sure we want to preserve that behaviour, regardless how {un,}likely ?
Certainly, thanks for spotting it.
Regards,
Tomeu
With the above fixed the patch is Reviewed-by: Emil Velikov emil.velikov@collabora.com
Regards, Emil