Re: [PATCH v9 2/4] drm: Add API for capturing frame CRCs
From: Emil Velikov
Date: Thu Oct 06 2016 - 08:21:26 EST
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