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

[Emil Velikov]

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.

>                         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 ?

With the above fixed the patch is
Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>

Regards,
Emil