Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755632AbcJFLOI (ORCPT ); Thu, 6 Oct 2016 07:14:08 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:33038 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751064AbcJFLN7 (ORCPT ); Thu, 6 Oct 2016 07:13:59 -0400 MIME-Version: 1.0 In-Reply-To: <1475744197-3828-3-git-send-email-tomeu.vizoso@collabora.com> References: <1475744197-3828-1-git-send-email-tomeu.vizoso@collabora.com> <1475744197-3828-3-git-send-email-tomeu.vizoso@collabora.com> From: Emil Velikov Date: Thu, 6 Oct 2016 12:13:57 +0100 Message-ID: Subject: Re: [PATCH v9 2/4] drm: Add API for capturing frame CRCs To: Tomeu Vizoso Cc: "Linux-Kernel@Vger. Kernel. Org" , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Sean Paul , Daniel Vetter , Thierry Reding , linux-doc@vger.kernel.org, David Airlie , ML dri-devel , Jonathan Corbet Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5326 Lines: 132 On 6 October 2016 at 09:56, Tomeu Vizoso 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 > --- > > 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 > #include > #include > -#include > +#include > > #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 Regards, Emil