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

[Tomeu Vizoso]

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.

>>                         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@xxxxxxxxxxxxx>
> 
> Regards,
> Emil
>