Message ID | 20221122190314.185015-2-mcanal@igalia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp2391390wrr; Tue, 22 Nov 2022 11:07:37 -0800 (PST) X-Google-Smtp-Source: AA0mqf4Vgw+xa7nPuU/UsGyaiu3STGADm3E67ffT4vLlt7QFR9xOMubvwjwbEPkM+aKC+tLkRIap X-Received: by 2002:a63:9318:0:b0:470:60a5:f822 with SMTP id b24-20020a639318000000b0047060a5f822mr4483664pge.302.1669144056923; Tue, 22 Nov 2022 11:07:36 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669144056; cv=none; d=google.com; s=arc-20160816; b=XGH1rUfr2Se64A7Jbx1F0PvKAy3avvtJbthfXvVuk8WMlrYhQ/U9BPfOqj34i8ShJ6 fWgjPvcZbajf79oaCVZhxXggtBm0mZ5t2ALv6mHlsHIU+vv+h1ZFfy0G4olS6Hh82bNR kIt2o9aiAA2AM5eWdvdJOhker/X6JwCAiL+8lHeXneD6Ld6P3jnQMazkYzhrpET9gM+u yhRLw2WYrur3tdbBR0BUQOVVmBttJnRtRklxlUr7hao8lzf/i+7neZhU4NndqJEMkasy n5ofseB3eA50MNAmj9iD/2yRvZu5fWIKVxDnTPRAm113oF/ooZ9uAtpqNJzzGWif71af l70Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=XS4whWcR6NeUrCBPhe59qw4wLDRUbcuOQVBRcdW3hpM=; b=yZquiRJApOWG6g+11JSe1tql19a+WZg4M85OzSDqDt2O6Wgun4uq4LeORnck3rSwF3 yQ1zk2TczBaQqsMQpzlfLnE0mURhN8eBEEu6iw3GGO/Z+nPf3O84atOuQnkr7xcMmEgg GaRv2Q1NpOUEzOa3kVTNj+xxsXOgw49HgENnDGyd6f39HpDh1zZDKyntq5OyqGK1+pw1 POruJWQ3Zo0kM6wsguVFkOIYHJDb4UUweUYXckRDx/4IgUPA7z1nhS1AM9Nim94Cg4q8 xdZmERGIGbno4nMOFVJAFMPdeUpcwrNJu4/Hp4pfH62nCuHJTR//k8YnsG8BzVk7pnVT C3xg== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@igalia.com header.s=20170329 header.b=dxKMAMuz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t8-20020a635348000000b0047704a39c4asi15230822pgl.504.2022.11.22.11.07.23; Tue, 22 Nov 2022 11:07:36 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@igalia.com header.s=20170329 header.b=dxKMAMuz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234517AbiKVTFB (ORCPT <rfc822;cjcooper78@gmail.com> + 99 others); Tue, 22 Nov 2022 14:05:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34450 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234428AbiKVTE7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 22 Nov 2022 14:04:59 -0500 Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5F80B82BE6 for <linux-kernel@vger.kernel.org>; Tue, 22 Nov 2022 11:04:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:MIME-Version:References: In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=XS4whWcR6NeUrCBPhe59qw4wLDRUbcuOQVBRcdW3hpM=; b=dxKMAMuz3ryWiEhfxJ3JX02HeX +iUu0stwEEWZ1DBUCmT3jPnKzX9Dujn/DIHgv6Wk9Ip9/JmdyMTvN78cQsUGdGCeixCaEBYx/M3bP SedzAvedQo7lFJf/s4AQyj4Dzh0h4IagXn/AuyTqqJr5eytyegU2gZ03743E3z/zkLkqaS+lu+K+g 1pjwCCk2UsKP2dj+aJ7al7WE1zVhNdlGS6bdFc+lhERWyOTXZmXR//bPLKncTSdH5P/BDMHmroAln EIh+ZnetL48klNgSxRsmn/YW2DV0ynuxmliKzcQxOoWusoUqJCdX9ynfbkenAkKLtvo9FOCb0fFGa M1t76Mhw==; Received: from [177.34.169.227] (helo=bowie..) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim) id 1oxYZX-007AP3-3l; Tue, 22 Nov 2022 20:04:43 +0100 From: =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Oded Gabbay <ogabbay@kernel.org> Cc: Melissa Wen <mwen@igalia.com>, =?utf-8?q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com>, Emma Anholt <emma@anholt.net>, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>, Wambui Karuga <wambui@karuga.org>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com>, Wambui Karuga <wambui.karugax@gmail.com> Subject: [PATCH 1/6] drm/debugfs: create device-centered debugfs functions Date: Tue, 22 Nov 2022 16:03:09 -0300 Message-Id: <20221122190314.185015-2-mcanal@igalia.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221122190314.185015-1-mcanal@igalia.com> References: <20221122190314.185015-1-mcanal@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750224399024109901?= X-GMAIL-MSGID: =?utf-8?q?1750224399024109901?= |
Series |
Introduce debugfs device-centered functions
|
|
Commit Message
Maíra Canal
Nov. 22, 2022, 7:03 p.m. UTC
Introduce the ability to track requests for the addition of DRM debugfs files at any time and have them added all at once during drm_dev_register(). Drivers can add DRM debugfs files to a device-managed list and, during drm_dev_register(), all added files will be created at once. Now, the drivers can use the functions drm_debugfs_add_file() and drm_debugfs_add_files() to create DRM debugfs files instead of using the drm_debugfs_create_files() function. Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c | 3 ++ include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ include/drm/drm_device.h | 15 +++++++ 4 files changed, 139 insertions(+)
Comments
On Tue, 22 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: > Introduce the ability to track requests for the addition of DRM debugfs > files at any time and have them added all at once during > drm_dev_register(). > > Drivers can add DRM debugfs files to a device-managed list and, during > drm_dev_register(), all added files will be created at once. > > Now, the drivers can use the functions drm_debugfs_add_file() and > drm_debugfs_add_files() to create DRM debugfs files instead of using the > drm_debugfs_create_files() function. > > Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> > Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_drv.c | 3 ++ > include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ > include/drm/drm_device.h | 15 +++++++ > 4 files changed, 139 insertions(+) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index ee445f4605ba..ca27c2b05051 100644 > --- a/drivers/gpu/drm/drm_debugfs.c > +++ b/drivers/gpu/drm/drm_debugfs.c > @@ -38,6 +38,7 @@ > #include <drm/drm_edid.h> > #include <drm/drm_file.h> > #include <drm/drm_gem.h> > +#include <drm/drm_managed.h> > > #include "drm_crtc_internal.h" > #include "drm_internal.h" > @@ -151,6 +152,21 @@ static int drm_debugfs_open(struct inode *inode, struct file *file) > return single_open(file, node->info_ent->show, node); > } > > +static int drm_debugfs_entry_open(struct inode *inode, struct file *file) > +{ > + struct drm_debugfs_entry *entry = inode->i_private; > + struct drm_debugfs_info *node = &entry->file; > + > + return single_open(file, node->show, entry); > +} > + > +static const struct file_operations drm_debugfs_entry_fops = { > + .owner = THIS_MODULE, > + .open = drm_debugfs_entry_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > +}; > > static const struct file_operations drm_debugfs_fops = { > .owner = THIS_MODULE, > @@ -207,6 +223,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, > struct dentry *root) > { > struct drm_device *dev = minor->dev; > + struct drm_debugfs_entry *entry; > char name[64]; > > INIT_LIST_HEAD(&minor->debugfs_list); > @@ -230,6 +247,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, > if (dev->driver->debugfs_init) > dev->driver->debugfs_init(minor); > > + list_for_each_entry(entry, &dev->debugfs_list, list) { > + debugfs_create_file(entry->file.name, S_IFREG | S_IRUGO, > + minor->debugfs_root, entry, &drm_debugfs_entry_fops); > + } > + > return 0; > } > > @@ -281,6 +303,60 @@ void drm_debugfs_cleanup(struct drm_minor *minor) > minor->debugfs_root = NULL; > } > > +/** > + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list > + * @dev: drm device for the ioctl > + * @name: debugfs file name > + * @show: show callback > + * @data: driver-private data, should not be device-specific > + * > + * Add a given file entry to the DRM device debugfs file list to be created on > + * drm_debugfs_init. > + */ > +int drm_debugfs_add_file(struct drm_device *dev, const char *name, > + int (*show)(struct seq_file*, void*), void *data) > +{ > + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); > + > + if (!entry) > + return -ENOMEM; > + > + entry->file.name = name; > + entry->file.show = show; > + entry->file.data = data; > + entry->dev = dev; > + > + mutex_lock(&dev->debugfs_mutex); > + list_add(&entry->list, &dev->debugfs_list); > + mutex_unlock(&dev->debugfs_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_debugfs_add_file); > + > +/** > + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list > + * @dev: drm device for the ioctl > + * @files: The array of files to create > + * @count: The number of files given > + * > + * Add a given set of debugfs files represented by an array of > + * &struct drm_debugfs_info in the DRM device debugfs file list. > + */ > +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) > +{ > + int i, ret = 0, err; > + > + for (i = 0; i < count; i++) { > + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); > + if (err) > + ret = err; > + } > + > + return ret; > +} > +EXPORT_SYMBOL(drm_debugfs_add_files); Do we want to add return values and error handling to debugfs related functions at all? BR, Jani. > + > static int connector_show(struct seq_file *m, void *data) > { > struct drm_connector *connector = m->private; > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 8214a0b1ab7f..803942008fcb 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) > mutex_destroy(&dev->clientlist_mutex); > mutex_destroy(&dev->filelist_mutex); > mutex_destroy(&dev->struct_mutex); > + mutex_destroy(&dev->debugfs_mutex); > drm_legacy_destroy_members(dev); > } > > @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, > INIT_LIST_HEAD(&dev->filelist_internal); > INIT_LIST_HEAD(&dev->clientlist); > INIT_LIST_HEAD(&dev->vblank_event_list); > + INIT_LIST_HEAD(&dev->debugfs_list); > > spin_lock_init(&dev->event_lock); > mutex_init(&dev->struct_mutex); > mutex_init(&dev->filelist_mutex); > mutex_init(&dev->clientlist_mutex); > mutex_init(&dev->master_mutex); > + mutex_init(&dev->debugfs_mutex); > > ret = drmm_add_action(dev, drm_dev_init_release, NULL); > if (ret) > diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h > index 2188dc83957f..c5684d6c5055 100644 > --- a/include/drm/drm_debugfs.h > +++ b/include/drm/drm_debugfs.h > @@ -79,12 +79,43 @@ struct drm_info_node { > struct dentry *dent; > }; > > +/** > + * struct drm_debugfs_info - debugfs info list entry > + * > + * This structure represents a debugfs file to be created by the drm > + * core. > + */ > +struct drm_debugfs_info { > + const char *name; > + int (*show)(struct seq_file*, void*); > + u32 driver_features; > + void *data; > +}; > + > +/** > + * struct drm_debugfs_entry - Per-device debugfs node structure > + * > + * This structure represents a debugfs file, as an instantiation of a &struct > + * drm_debugfs_info on a &struct drm_device. > + */ > +struct drm_debugfs_entry { > + struct drm_device *dev; > + struct drm_debugfs_info file; > + struct list_head list; > +}; > + > #if defined(CONFIG_DEBUG_FS) > void drm_debugfs_create_files(const struct drm_info_list *files, > int count, struct dentry *root, > struct drm_minor *minor); > int drm_debugfs_remove_files(const struct drm_info_list *files, > int count, struct drm_minor *minor); > + > +int drm_debugfs_add_file(struct drm_device *dev, const char *name, > + int (*show)(struct seq_file*, void*), void *data); > + > +int drm_debugfs_add_files(struct drm_device *dev, > + const struct drm_debugfs_info *files, int count); > #else > static inline void drm_debugfs_create_files(const struct drm_info_list *files, > int count, struct dentry *root, > @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, > { > return 0; > } > + > +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, > + int (*show)(struct seq_file*, void*), > + void *data) > +{ > + return 0; > +} > + > +static inline int drm_debugfs_add_files(struct drm_device *dev, > + const struct drm_debugfs_info *files, > + int count) > +{ > + return 0; > +} > #endif > > #endif /* _DRM_DEBUGFS_H_ */ > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 9923c7a6885e..fa6af1d57929 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -295,6 +295,21 @@ struct drm_device { > */ > struct drm_fb_helper *fb_helper; > > + /** > + * @debugfs_mutex: > + * > + * Protects &debugfs_list access. > + */ > + struct mutex debugfs_mutex; > + > + /** > + * @debugfs_list: > + * > + * List of debugfs files to be created by the DRM device. The files > + * must be added during drm_dev_register(). > + */ > + struct list_head debugfs_list; > + > /* Everything below here is for legacy driver, never use! */ > /* private: */ > #if IS_ENABLED(CONFIG_DRM_LEGACY)
Hi Jani, On 11/23/22 08:06, Jani Nikula wrote: > On Tue, 22 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >> Introduce the ability to track requests for the addition of DRM debugfs >> files at any time and have them added all at once during >> drm_dev_register(). >> >> Drivers can add DRM debugfs files to a device-managed list and, during >> drm_dev_register(), all added files will be created at once. >> >> Now, the drivers can use the functions drm_debugfs_add_file() and >> drm_debugfs_add_files() to create DRM debugfs files instead of using the >> drm_debugfs_create_files() function. >> >> Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> >> Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_drv.c | 3 ++ >> include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ >> include/drm/drm_device.h | 15 +++++++ >> 4 files changed, 139 insertions(+) >>>> +/** >> + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list >> + * @dev: drm device for the ioctl >> + * @name: debugfs file name >> + * @show: show callback >> + * @data: driver-private data, should not be device-specific >> + * >> + * Add a given file entry to the DRM device debugfs file list to be created on >> + * drm_debugfs_init. >> + */ >> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >> + int (*show)(struct seq_file*, void*), void *data) >> +{ >> + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); >> + >> + if (!entry) >> + return -ENOMEM; >> + >> + entry->file.name = name; >> + entry->file.show = show; >> + entry->file.data = data; >> + entry->dev = dev; >> + >> + mutex_lock(&dev->debugfs_mutex); >> + list_add(&entry->list, &dev->debugfs_list); >> + mutex_unlock(&dev->debugfs_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_debugfs_add_file); >> + >> +/** >> + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list >> + * @dev: drm device for the ioctl >> + * @files: The array of files to create >> + * @count: The number of files given >> + * >> + * Add a given set of debugfs files represented by an array of >> + * &struct drm_debugfs_info in the DRM device debugfs file list. >> + */ >> +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) >> +{ >> + int i, ret = 0, err; >> + >> + for (i = 0; i < count; i++) { >> + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); >> + if (err) >> + ret = err; >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_debugfs_add_files); > > Do we want to add return values and error handling to debugfs related > functions at all? Drivers such as vc4 can use the return values from debugfs-related functions for error handling. Although the return values are not explicitly necessary, some drivers can benefit from them for error handling. Best Regards, - Maíra Canal > > BR, > Jani. > > >> + >> static int connector_show(struct seq_file *m, void *data) >> { >> struct drm_connector *connector = m->private; >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 8214a0b1ab7f..803942008fcb 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >> mutex_destroy(&dev->clientlist_mutex); >> mutex_destroy(&dev->filelist_mutex); >> mutex_destroy(&dev->struct_mutex); >> + mutex_destroy(&dev->debugfs_mutex); >> drm_legacy_destroy_members(dev); >> } >> >> @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, >> INIT_LIST_HEAD(&dev->filelist_internal); >> INIT_LIST_HEAD(&dev->clientlist); >> INIT_LIST_HEAD(&dev->vblank_event_list); >> + INIT_LIST_HEAD(&dev->debugfs_list); >> >> spin_lock_init(&dev->event_lock); >> mutex_init(&dev->struct_mutex); >> mutex_init(&dev->filelist_mutex); >> mutex_init(&dev->clientlist_mutex); >> mutex_init(&dev->master_mutex); >> + mutex_init(&dev->debugfs_mutex); >> >> ret = drmm_add_action(dev, drm_dev_init_release, NULL); >> if (ret) >> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h >> index 2188dc83957f..c5684d6c5055 100644 >> --- a/include/drm/drm_debugfs.h >> +++ b/include/drm/drm_debugfs.h >> @@ -79,12 +79,43 @@ struct drm_info_node { >> struct dentry *dent; >> }; >> >> +/** >> + * struct drm_debugfs_info - debugfs info list entry >> + * >> + * This structure represents a debugfs file to be created by the drm >> + * core. >> + */ >> +struct drm_debugfs_info { >> + const char *name; >> + int (*show)(struct seq_file*, void*); >> + u32 driver_features; >> + void *data; >> +}; >> + >> +/** >> + * struct drm_debugfs_entry - Per-device debugfs node structure >> + * >> + * This structure represents a debugfs file, as an instantiation of a &struct >> + * drm_debugfs_info on a &struct drm_device. >> + */ >> +struct drm_debugfs_entry { >> + struct drm_device *dev; >> + struct drm_debugfs_info file; >> + struct list_head list; >> +}; >> + >> #if defined(CONFIG_DEBUG_FS) >> void drm_debugfs_create_files(const struct drm_info_list *files, >> int count, struct dentry *root, >> struct drm_minor *minor); >> int drm_debugfs_remove_files(const struct drm_info_list *files, >> int count, struct drm_minor *minor); >> + >> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >> + int (*show)(struct seq_file*, void*), void *data); >> + >> +int drm_debugfs_add_files(struct drm_device *dev, >> + const struct drm_debugfs_info *files, int count); >> #else >> static inline void drm_debugfs_create_files(const struct drm_info_list *files, >> int count, struct dentry *root, >> @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, >> { >> return 0; >> } >> + >> +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, >> + int (*show)(struct seq_file*, void*), >> + void *data) >> +{ >> + return 0; >> +} >> + >> +static inline int drm_debugfs_add_files(struct drm_device *dev, >> + const struct drm_debugfs_info *files, >> + int count) >> +{ >> + return 0; >> +} >> #endif >> >> #endif /* _DRM_DEBUGFS_H_ */ >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index 9923c7a6885e..fa6af1d57929 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -295,6 +295,21 @@ struct drm_device { >> */ >> struct drm_fb_helper *fb_helper; >> >> + /** >> + * @debugfs_mutex: >> + * >> + * Protects &debugfs_list access. >> + */ >> + struct mutex debugfs_mutex; >> + >> + /** >> + * @debugfs_list: >> + * >> + * List of debugfs files to be created by the DRM device. The files >> + * must be added during drm_dev_register(). >> + */ >> + struct list_head debugfs_list; >> + >> /* Everything below here is for legacy driver, never use! */ >> /* private: */ >> #if IS_ENABLED(CONFIG_DRM_LEGACY) >
On Wed, 23 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: > Hi Jani, > > On 11/23/22 08:06, Jani Nikula wrote: >> On Tue, 22 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >>> Introduce the ability to track requests for the addition of DRM debugfs >>> files at any time and have them added all at once during >>> drm_dev_register(). >>> >>> Drivers can add DRM debugfs files to a device-managed list and, during >>> drm_dev_register(), all added files will be created at once. >>> >>> Now, the drivers can use the functions drm_debugfs_add_file() and >>> drm_debugfs_add_files() to create DRM debugfs files instead of using the >>> drm_debugfs_create_files() function. >>> >>> Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> >>> Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> >>> Signed-off-by: Maíra Canal <mcanal@igalia.com> >>> --- >>> drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/drm_drv.c | 3 ++ >>> include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ >>> include/drm/drm_device.h | 15 +++++++ >>> 4 files changed, 139 insertions(+) >>>>> +/** >>> + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list >>> + * @dev: drm device for the ioctl >>> + * @name: debugfs file name >>> + * @show: show callback >>> + * @data: driver-private data, should not be device-specific >>> + * >>> + * Add a given file entry to the DRM device debugfs file list to be created on >>> + * drm_debugfs_init. >>> + */ >>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>> + int (*show)(struct seq_file*, void*), void *data) >>> +{ >>> + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); >>> + >>> + if (!entry) >>> + return -ENOMEM; >>> + >>> + entry->file.name = name; >>> + entry->file.show = show; >>> + entry->file.data = data; >>> + entry->dev = dev; >>> + >>> + mutex_lock(&dev->debugfs_mutex); >>> + list_add(&entry->list, &dev->debugfs_list); >>> + mutex_unlock(&dev->debugfs_mutex); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(drm_debugfs_add_file); >>> + >>> +/** >>> + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list >>> + * @dev: drm device for the ioctl >>> + * @files: The array of files to create >>> + * @count: The number of files given >>> + * >>> + * Add a given set of debugfs files represented by an array of >>> + * &struct drm_debugfs_info in the DRM device debugfs file list. >>> + */ >>> +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) >>> +{ >>> + int i, ret = 0, err; >>> + >>> + for (i = 0; i < count; i++) { >>> + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); >>> + if (err) >>> + ret = err; >>> + } >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL(drm_debugfs_add_files); >> >> Do we want to add return values and error handling to debugfs related >> functions at all? > > Drivers such as vc4 can use the return values from debugfs-related > functions for error handling. Although the return values are not > explicitly necessary, some drivers can benefit from them for error handling. Arguably they should cease to do error handling on debugfs failures too. No driver should stop probe if debugfs fails, and that's been the direction for years. BR, Jani. > > Best Regards, > - Maíra Canal > >> >> BR, >> Jani. >> >> >>> + >>> static int connector_show(struct seq_file *m, void *data) >>> { >>> struct drm_connector *connector = m->private; >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 8214a0b1ab7f..803942008fcb 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >>> @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >>> mutex_destroy(&dev->clientlist_mutex); >>> mutex_destroy(&dev->filelist_mutex); >>> mutex_destroy(&dev->struct_mutex); >>> + mutex_destroy(&dev->debugfs_mutex); >>> drm_legacy_destroy_members(dev); >>> } >>> >>> @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, >>> INIT_LIST_HEAD(&dev->filelist_internal); >>> INIT_LIST_HEAD(&dev->clientlist); >>> INIT_LIST_HEAD(&dev->vblank_event_list); >>> + INIT_LIST_HEAD(&dev->debugfs_list); >>> >>> spin_lock_init(&dev->event_lock); >>> mutex_init(&dev->struct_mutex); >>> mutex_init(&dev->filelist_mutex); >>> mutex_init(&dev->clientlist_mutex); >>> mutex_init(&dev->master_mutex); >>> + mutex_init(&dev->debugfs_mutex); >>> >>> ret = drmm_add_action(dev, drm_dev_init_release, NULL); >>> if (ret) >>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h >>> index 2188dc83957f..c5684d6c5055 100644 >>> --- a/include/drm/drm_debugfs.h >>> +++ b/include/drm/drm_debugfs.h >>> @@ -79,12 +79,43 @@ struct drm_info_node { >>> struct dentry *dent; >>> }; >>> >>> +/** >>> + * struct drm_debugfs_info - debugfs info list entry >>> + * >>> + * This structure represents a debugfs file to be created by the drm >>> + * core. >>> + */ >>> +struct drm_debugfs_info { >>> + const char *name; >>> + int (*show)(struct seq_file*, void*); >>> + u32 driver_features; >>> + void *data; >>> +}; >>> + >>> +/** >>> + * struct drm_debugfs_entry - Per-device debugfs node structure >>> + * >>> + * This structure represents a debugfs file, as an instantiation of a &struct >>> + * drm_debugfs_info on a &struct drm_device. >>> + */ >>> +struct drm_debugfs_entry { >>> + struct drm_device *dev; >>> + struct drm_debugfs_info file; >>> + struct list_head list; >>> +}; >>> + >>> #if defined(CONFIG_DEBUG_FS) >>> void drm_debugfs_create_files(const struct drm_info_list *files, >>> int count, struct dentry *root, >>> struct drm_minor *minor); >>> int drm_debugfs_remove_files(const struct drm_info_list *files, >>> int count, struct drm_minor *minor); >>> + >>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>> + int (*show)(struct seq_file*, void*), void *data); >>> + >>> +int drm_debugfs_add_files(struct drm_device *dev, >>> + const struct drm_debugfs_info *files, int count); >>> #else >>> static inline void drm_debugfs_create_files(const struct drm_info_list *files, >>> int count, struct dentry *root, >>> @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, >>> { >>> return 0; >>> } >>> + >>> +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>> + int (*show)(struct seq_file*, void*), >>> + void *data) >>> +{ >>> + return 0; >>> +} >>> + >>> +static inline int drm_debugfs_add_files(struct drm_device *dev, >>> + const struct drm_debugfs_info *files, >>> + int count) >>> +{ >>> + return 0; >>> +} >>> #endif >>> >>> #endif /* _DRM_DEBUGFS_H_ */ >>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >>> index 9923c7a6885e..fa6af1d57929 100644 >>> --- a/include/drm/drm_device.h >>> +++ b/include/drm/drm_device.h >>> @@ -295,6 +295,21 @@ struct drm_device { >>> */ >>> struct drm_fb_helper *fb_helper; >>> >>> + /** >>> + * @debugfs_mutex: >>> + * >>> + * Protects &debugfs_list access. >>> + */ >>> + struct mutex debugfs_mutex; >>> + >>> + /** >>> + * @debugfs_list: >>> + * >>> + * List of debugfs files to be created by the DRM device. The files >>> + * must be added during drm_dev_register(). >>> + */ >>> + struct list_head debugfs_list; >>> + >>> /* Everything below here is for legacy driver, never use! */ >>> /* private: */ >>> #if IS_ENABLED(CONFIG_DRM_LEGACY) >>
On 11/23/22 08:59, Jani Nikula wrote: > On Wed, 23 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >> Hi Jani, >> >> On 11/23/22 08:06, Jani Nikula wrote: >>> On Tue, 22 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >>>> Introduce the ability to track requests for the addition of DRM debugfs >>>> files at any time and have them added all at once during >>>> drm_dev_register(). >>>> >>>> Drivers can add DRM debugfs files to a device-managed list and, during >>>> drm_dev_register(), all added files will be created at once. >>>> >>>> Now, the drivers can use the functions drm_debugfs_add_file() and >>>> drm_debugfs_add_files() to create DRM debugfs files instead of using the >>>> drm_debugfs_create_files() function. >>>> >>>> Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> >>>> Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com> >>>> --- >>>> drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/drm_drv.c | 3 ++ >>>> include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ >>>> include/drm/drm_device.h | 15 +++++++ >>>> 4 files changed, 139 insertions(+) >>>>>> +/** >>>> + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list >>>> + * @dev: drm device for the ioctl >>>> + * @name: debugfs file name >>>> + * @show: show callback >>>> + * @data: driver-private data, should not be device-specific >>>> + * >>>> + * Add a given file entry to the DRM device debugfs file list to be created on >>>> + * drm_debugfs_init. >>>> + */ >>>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>> + int (*show)(struct seq_file*, void*), void *data) >>>> +{ >>>> + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); >>>> + >>>> + if (!entry) >>>> + return -ENOMEM; >>>> + >>>> + entry->file.name = name; >>>> + entry->file.show = show; >>>> + entry->file.data = data; >>>> + entry->dev = dev; >>>> + >>>> + mutex_lock(&dev->debugfs_mutex); >>>> + list_add(&entry->list, &dev->debugfs_list); >>>> + mutex_unlock(&dev->debugfs_mutex); >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(drm_debugfs_add_file); >>>> + >>>> +/** >>>> + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list >>>> + * @dev: drm device for the ioctl >>>> + * @files: The array of files to create >>>> + * @count: The number of files given >>>> + * >>>> + * Add a given set of debugfs files represented by an array of >>>> + * &struct drm_debugfs_info in the DRM device debugfs file list. >>>> + */ >>>> +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) >>>> +{ >>>> + int i, ret = 0, err; >>>> + >>>> + for (i = 0; i < count; i++) { >>>> + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); >>>> + if (err) >>>> + ret = err; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL(drm_debugfs_add_files); >>> >>> Do we want to add return values and error handling to debugfs related >>> functions at all? >> >> Drivers such as vc4 can use the return values from debugfs-related >> functions for error handling. Although the return values are not >> explicitly necessary, some drivers can benefit from them for error handling. > > Arguably they should cease to do error handling on debugfs failures > too. No driver should stop probe if debugfs fails, and that's been the > direction for years. Is it not even reasonable to return errors only to create drm_WARN_ON when the creation of debugfs files fails? Currently, vc4 doesn't stop to probe if debugfs fails, but only creates some warnings to let the user knows that it failed. Best Regards, - Maíra Canal > > BR, > Jani. > >> >> Best Regards, >> - Maíra Canal >> >>> >>> BR, >>> Jani. >>> >>> >>>> + >>>> static int connector_show(struct seq_file *m, void *data) >>>> { >>>> struct drm_connector *connector = m->private; >>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>> index 8214a0b1ab7f..803942008fcb 100644 >>>> --- a/drivers/gpu/drm/drm_drv.c >>>> +++ b/drivers/gpu/drm/drm_drv.c >>>> @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >>>> mutex_destroy(&dev->clientlist_mutex); >>>> mutex_destroy(&dev->filelist_mutex); >>>> mutex_destroy(&dev->struct_mutex); >>>> + mutex_destroy(&dev->debugfs_mutex); >>>> drm_legacy_destroy_members(dev); >>>> } >>>> >>>> @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, >>>> INIT_LIST_HEAD(&dev->filelist_internal); >>>> INIT_LIST_HEAD(&dev->clientlist); >>>> INIT_LIST_HEAD(&dev->vblank_event_list); >>>> + INIT_LIST_HEAD(&dev->debugfs_list); >>>> >>>> spin_lock_init(&dev->event_lock); >>>> mutex_init(&dev->struct_mutex); >>>> mutex_init(&dev->filelist_mutex); >>>> mutex_init(&dev->clientlist_mutex); >>>> mutex_init(&dev->master_mutex); >>>> + mutex_init(&dev->debugfs_mutex); >>>> >>>> ret = drmm_add_action(dev, drm_dev_init_release, NULL); >>>> if (ret) >>>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h >>>> index 2188dc83957f..c5684d6c5055 100644 >>>> --- a/include/drm/drm_debugfs.h >>>> +++ b/include/drm/drm_debugfs.h >>>> @@ -79,12 +79,43 @@ struct drm_info_node { >>>> struct dentry *dent; >>>> }; >>>> >>>> +/** >>>> + * struct drm_debugfs_info - debugfs info list entry >>>> + * >>>> + * This structure represents a debugfs file to be created by the drm >>>> + * core. >>>> + */ >>>> +struct drm_debugfs_info { >>>> + const char *name; >>>> + int (*show)(struct seq_file*, void*); >>>> + u32 driver_features; >>>> + void *data; >>>> +}; >>>> + >>>> +/** >>>> + * struct drm_debugfs_entry - Per-device debugfs node structure >>>> + * >>>> + * This structure represents a debugfs file, as an instantiation of a &struct >>>> + * drm_debugfs_info on a &struct drm_device. >>>> + */ >>>> +struct drm_debugfs_entry { >>>> + struct drm_device *dev; >>>> + struct drm_debugfs_info file; >>>> + struct list_head list; >>>> +}; >>>> + >>>> #if defined(CONFIG_DEBUG_FS) >>>> void drm_debugfs_create_files(const struct drm_info_list *files, >>>> int count, struct dentry *root, >>>> struct drm_minor *minor); >>>> int drm_debugfs_remove_files(const struct drm_info_list *files, >>>> int count, struct drm_minor *minor); >>>> + >>>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>> + int (*show)(struct seq_file*, void*), void *data); >>>> + >>>> +int drm_debugfs_add_files(struct drm_device *dev, >>>> + const struct drm_debugfs_info *files, int count); >>>> #else >>>> static inline void drm_debugfs_create_files(const struct drm_info_list *files, >>>> int count, struct dentry *root, >>>> @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, >>>> { >>>> return 0; >>>> } >>>> + >>>> +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>> + int (*show)(struct seq_file*, void*), >>>> + void *data) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> +static inline int drm_debugfs_add_files(struct drm_device *dev, >>>> + const struct drm_debugfs_info *files, >>>> + int count) >>>> +{ >>>> + return 0; >>>> +} >>>> #endif >>>> >>>> #endif /* _DRM_DEBUGFS_H_ */ >>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >>>> index 9923c7a6885e..fa6af1d57929 100644 >>>> --- a/include/drm/drm_device.h >>>> +++ b/include/drm/drm_device.h >>>> @@ -295,6 +295,21 @@ struct drm_device { >>>> */ >>>> struct drm_fb_helper *fb_helper; >>>> >>>> + /** >>>> + * @debugfs_mutex: >>>> + * >>>> + * Protects &debugfs_list access. >>>> + */ >>>> + struct mutex debugfs_mutex; >>>> + >>>> + /** >>>> + * @debugfs_list: >>>> + * >>>> + * List of debugfs files to be created by the DRM device. The files >>>> + * must be added during drm_dev_register(). >>>> + */ >>>> + struct list_head debugfs_list; >>>> + >>>> /* Everything below here is for legacy driver, never use! */ >>>> /* private: */ >>>> #if IS_ENABLED(CONFIG_DRM_LEGACY) >>> >
On Wed, 23 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: > On 11/23/22 08:59, Jani Nikula wrote: >> On Wed, 23 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >>> Hi Jani, >>> >>> On 11/23/22 08:06, Jani Nikula wrote: >>>> On Tue, 22 Nov 2022, Maíra Canal <mcanal@igalia.com> wrote: >>>>> Introduce the ability to track requests for the addition of DRM debugfs >>>>> files at any time and have them added all at once during >>>>> drm_dev_register(). >>>>> >>>>> Drivers can add DRM debugfs files to a device-managed list and, during >>>>> drm_dev_register(), all added files will be created at once. >>>>> >>>>> Now, the drivers can use the functions drm_debugfs_add_file() and >>>>> drm_debugfs_add_files() to create DRM debugfs files instead of using the >>>>> drm_debugfs_create_files() function. >>>>> >>>>> Co-developed-by: Wambui Karuga <wambui.karugax@gmail.com> >>>>> Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com> >>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com> >>>>> --- >>>>> drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++ >>>>> drivers/gpu/drm/drm_drv.c | 3 ++ >>>>> include/drm/drm_debugfs.h | 45 +++++++++++++++++++++ >>>>> include/drm/drm_device.h | 15 +++++++ >>>>> 4 files changed, 139 insertions(+) >>>>>>> +/** >>>>> + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list >>>>> + * @dev: drm device for the ioctl >>>>> + * @name: debugfs file name >>>>> + * @show: show callback >>>>> + * @data: driver-private data, should not be device-specific >>>>> + * >>>>> + * Add a given file entry to the DRM device debugfs file list to be created on >>>>> + * drm_debugfs_init. >>>>> + */ >>>>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>>> + int (*show)(struct seq_file*, void*), void *data) >>>>> +{ >>>>> + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); >>>>> + >>>>> + if (!entry) >>>>> + return -ENOMEM; >>>>> + >>>>> + entry->file.name = name; >>>>> + entry->file.show = show; >>>>> + entry->file.data = data; >>>>> + entry->dev = dev; >>>>> + >>>>> + mutex_lock(&dev->debugfs_mutex); >>>>> + list_add(&entry->list, &dev->debugfs_list); >>>>> + mutex_unlock(&dev->debugfs_mutex); >>>>> + >>>>> + return 0; >>>>> +} >>>>> +EXPORT_SYMBOL(drm_debugfs_add_file); >>>>> + >>>>> +/** >>>>> + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list >>>>> + * @dev: drm device for the ioctl >>>>> + * @files: The array of files to create >>>>> + * @count: The number of files given >>>>> + * >>>>> + * Add a given set of debugfs files represented by an array of >>>>> + * &struct drm_debugfs_info in the DRM device debugfs file list. >>>>> + */ >>>>> +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) >>>>> +{ >>>>> + int i, ret = 0, err; >>>>> + >>>>> + for (i = 0; i < count; i++) { >>>>> + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); >>>>> + if (err) >>>>> + ret = err; >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL(drm_debugfs_add_files); >>>> >>>> Do we want to add return values and error handling to debugfs related >>>> functions at all? >>> >>> Drivers such as vc4 can use the return values from debugfs-related >>> functions for error handling. Although the return values are not >>> explicitly necessary, some drivers can benefit from them for error handling. >> >> Arguably they should cease to do error handling on debugfs failures >> too. No driver should stop probe if debugfs fails, and that's been the >> direction for years. > > Is it not even reasonable to return errors only to create drm_WARN_ON > when the creation of debugfs files fails? Currently, vc4 doesn't stop to > probe if debugfs fails, but only creates some warnings to let the user > knows that it failed. First, the only failure mode currently is -ENOMEM, for which you should never warn or debug log anyway. Second, Greg's been telling folks for years to not care about the results of debugfs creation calls [1]. Sure, this is slightly different in that it's preparation for making those calls, but for practical purposes the end result is the same, right? If there were other failure modes than -ENOMEM, I guess you could debug log them from within drm_debugfs_add_file(), but what does the driver do with the return code? Most drivers don't care anyway, and IMO they shouldn't care. BR, Jani. [1] https://lore.kernel.org/r/YWAmZdRwnAt6wh9B@kroah.com > > Best Regards, > - Maíra Canal > >> >> BR, >> Jani. >> >>> >>> Best Regards, >>> - Maíra Canal >>> >>>> >>>> BR, >>>> Jani. >>>> >>>> >>>>> + >>>>> static int connector_show(struct seq_file *m, void *data) >>>>> { >>>>> struct drm_connector *connector = m->private; >>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>>> index 8214a0b1ab7f..803942008fcb 100644 >>>>> --- a/drivers/gpu/drm/drm_drv.c >>>>> +++ b/drivers/gpu/drm/drm_drv.c >>>>> @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) >>>>> mutex_destroy(&dev->clientlist_mutex); >>>>> mutex_destroy(&dev->filelist_mutex); >>>>> mutex_destroy(&dev->struct_mutex); >>>>> + mutex_destroy(&dev->debugfs_mutex); >>>>> drm_legacy_destroy_members(dev); >>>>> } >>>>> >>>>> @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, >>>>> INIT_LIST_HEAD(&dev->filelist_internal); >>>>> INIT_LIST_HEAD(&dev->clientlist); >>>>> INIT_LIST_HEAD(&dev->vblank_event_list); >>>>> + INIT_LIST_HEAD(&dev->debugfs_list); >>>>> >>>>> spin_lock_init(&dev->event_lock); >>>>> mutex_init(&dev->struct_mutex); >>>>> mutex_init(&dev->filelist_mutex); >>>>> mutex_init(&dev->clientlist_mutex); >>>>> mutex_init(&dev->master_mutex); >>>>> + mutex_init(&dev->debugfs_mutex); >>>>> >>>>> ret = drmm_add_action(dev, drm_dev_init_release, NULL); >>>>> if (ret) >>>>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h >>>>> index 2188dc83957f..c5684d6c5055 100644 >>>>> --- a/include/drm/drm_debugfs.h >>>>> +++ b/include/drm/drm_debugfs.h >>>>> @@ -79,12 +79,43 @@ struct drm_info_node { >>>>> struct dentry *dent; >>>>> }; >>>>> >>>>> +/** >>>>> + * struct drm_debugfs_info - debugfs info list entry >>>>> + * >>>>> + * This structure represents a debugfs file to be created by the drm >>>>> + * core. >>>>> + */ >>>>> +struct drm_debugfs_info { >>>>> + const char *name; >>>>> + int (*show)(struct seq_file*, void*); >>>>> + u32 driver_features; >>>>> + void *data; >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct drm_debugfs_entry - Per-device debugfs node structure >>>>> + * >>>>> + * This structure represents a debugfs file, as an instantiation of a &struct >>>>> + * drm_debugfs_info on a &struct drm_device. >>>>> + */ >>>>> +struct drm_debugfs_entry { >>>>> + struct drm_device *dev; >>>>> + struct drm_debugfs_info file; >>>>> + struct list_head list; >>>>> +}; >>>>> + >>>>> #if defined(CONFIG_DEBUG_FS) >>>>> void drm_debugfs_create_files(const struct drm_info_list *files, >>>>> int count, struct dentry *root, >>>>> struct drm_minor *minor); >>>>> int drm_debugfs_remove_files(const struct drm_info_list *files, >>>>> int count, struct drm_minor *minor); >>>>> + >>>>> +int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>>> + int (*show)(struct seq_file*, void*), void *data); >>>>> + >>>>> +int drm_debugfs_add_files(struct drm_device *dev, >>>>> + const struct drm_debugfs_info *files, int count); >>>>> #else >>>>> static inline void drm_debugfs_create_files(const struct drm_info_list *files, >>>>> int count, struct dentry *root, >>>>> @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, >>>>> { >>>>> return 0; >>>>> } >>>>> + >>>>> +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, >>>>> + int (*show)(struct seq_file*, void*), >>>>> + void *data) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static inline int drm_debugfs_add_files(struct drm_device *dev, >>>>> + const struct drm_debugfs_info *files, >>>>> + int count) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> #endif >>>>> >>>>> #endif /* _DRM_DEBUGFS_H_ */ >>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >>>>> index 9923c7a6885e..fa6af1d57929 100644 >>>>> --- a/include/drm/drm_device.h >>>>> +++ b/include/drm/drm_device.h >>>>> @@ -295,6 +295,21 @@ struct drm_device { >>>>> */ >>>>> struct drm_fb_helper *fb_helper; >>>>> >>>>> + /** >>>>> + * @debugfs_mutex: >>>>> + * >>>>> + * Protects &debugfs_list access. >>>>> + */ >>>>> + struct mutex debugfs_mutex; >>>>> + >>>>> + /** >>>>> + * @debugfs_list: >>>>> + * >>>>> + * List of debugfs files to be created by the DRM device. The files >>>>> + * must be added during drm_dev_register(). >>>>> + */ >>>>> + struct list_head debugfs_list; >>>>> + >>>>> /* Everything below here is for legacy driver, never use! */ >>>>> /* private: */ >>>>> #if IS_ENABLED(CONFIG_DRM_LEGACY) >>>> >>
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index ee445f4605ba..ca27c2b05051 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -38,6 +38,7 @@ #include <drm/drm_edid.h> #include <drm/drm_file.h> #include <drm/drm_gem.h> +#include <drm/drm_managed.h> #include "drm_crtc_internal.h" #include "drm_internal.h" @@ -151,6 +152,21 @@ static int drm_debugfs_open(struct inode *inode, struct file *file) return single_open(file, node->info_ent->show, node); } +static int drm_debugfs_entry_open(struct inode *inode, struct file *file) +{ + struct drm_debugfs_entry *entry = inode->i_private; + struct drm_debugfs_info *node = &entry->file; + + return single_open(file, node->show, entry); +} + +static const struct file_operations drm_debugfs_entry_fops = { + .owner = THIS_MODULE, + .open = drm_debugfs_entry_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; static const struct file_operations drm_debugfs_fops = { .owner = THIS_MODULE, @@ -207,6 +223,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, struct dentry *root) { struct drm_device *dev = minor->dev; + struct drm_debugfs_entry *entry; char name[64]; INIT_LIST_HEAD(&minor->debugfs_list); @@ -230,6 +247,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, if (dev->driver->debugfs_init) dev->driver->debugfs_init(minor); + list_for_each_entry(entry, &dev->debugfs_list, list) { + debugfs_create_file(entry->file.name, S_IFREG | S_IRUGO, + minor->debugfs_root, entry, &drm_debugfs_entry_fops); + } + return 0; } @@ -281,6 +303,60 @@ void drm_debugfs_cleanup(struct drm_minor *minor) minor->debugfs_root = NULL; } +/** + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list + * @dev: drm device for the ioctl + * @name: debugfs file name + * @show: show callback + * @data: driver-private data, should not be device-specific + * + * Add a given file entry to the DRM device debugfs file list to be created on + * drm_debugfs_init. + */ +int drm_debugfs_add_file(struct drm_device *dev, const char *name, + int (*show)(struct seq_file*, void*), void *data) +{ + struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL); + + if (!entry) + return -ENOMEM; + + entry->file.name = name; + entry->file.show = show; + entry->file.data = data; + entry->dev = dev; + + mutex_lock(&dev->debugfs_mutex); + list_add(&entry->list, &dev->debugfs_list); + mutex_unlock(&dev->debugfs_mutex); + + return 0; +} +EXPORT_SYMBOL(drm_debugfs_add_file); + +/** + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list + * @dev: drm device for the ioctl + * @files: The array of files to create + * @count: The number of files given + * + * Add a given set of debugfs files represented by an array of + * &struct drm_debugfs_info in the DRM device debugfs file list. + */ +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count) +{ + int i, ret = 0, err; + + for (i = 0; i < count; i++) { + err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data); + if (err) + ret = err; + } + + return ret; +} +EXPORT_SYMBOL(drm_debugfs_add_files); + static int connector_show(struct seq_file *m, void *data) { struct drm_connector *connector = m->private; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8214a0b1ab7f..803942008fcb 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); + mutex_destroy(&dev->debugfs_mutex); drm_legacy_destroy_members(dev); } @@ -608,12 +609,14 @@ static int drm_dev_init(struct drm_device *dev, INIT_LIST_HEAD(&dev->filelist_internal); INIT_LIST_HEAD(&dev->clientlist); INIT_LIST_HEAD(&dev->vblank_event_list); + INIT_LIST_HEAD(&dev->debugfs_list); spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->filelist_mutex); mutex_init(&dev->clientlist_mutex); mutex_init(&dev->master_mutex); + mutex_init(&dev->debugfs_mutex); ret = drmm_add_action(dev, drm_dev_init_release, NULL); if (ret) diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h index 2188dc83957f..c5684d6c5055 100644 --- a/include/drm/drm_debugfs.h +++ b/include/drm/drm_debugfs.h @@ -79,12 +79,43 @@ struct drm_info_node { struct dentry *dent; }; +/** + * struct drm_debugfs_info - debugfs info list entry + * + * This structure represents a debugfs file to be created by the drm + * core. + */ +struct drm_debugfs_info { + const char *name; + int (*show)(struct seq_file*, void*); + u32 driver_features; + void *data; +}; + +/** + * struct drm_debugfs_entry - Per-device debugfs node structure + * + * This structure represents a debugfs file, as an instantiation of a &struct + * drm_debugfs_info on a &struct drm_device. + */ +struct drm_debugfs_entry { + struct drm_device *dev; + struct drm_debugfs_info file; + struct list_head list; +}; + #if defined(CONFIG_DEBUG_FS) void drm_debugfs_create_files(const struct drm_info_list *files, int count, struct dentry *root, struct drm_minor *minor); int drm_debugfs_remove_files(const struct drm_info_list *files, int count, struct drm_minor *minor); + +int drm_debugfs_add_file(struct drm_device *dev, const char *name, + int (*show)(struct seq_file*, void*), void *data); + +int drm_debugfs_add_files(struct drm_device *dev, + const struct drm_debugfs_info *files, int count); #else static inline void drm_debugfs_create_files(const struct drm_info_list *files, int count, struct dentry *root, @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files, { return 0; } + +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name, + int (*show)(struct seq_file*, void*), + void *data) +{ + return 0; +} + +static inline int drm_debugfs_add_files(struct drm_device *dev, + const struct drm_debugfs_info *files, + int count) +{ + return 0; +} #endif #endif /* _DRM_DEBUGFS_H_ */ diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 9923c7a6885e..fa6af1d57929 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -295,6 +295,21 @@ struct drm_device { */ struct drm_fb_helper *fb_helper; + /** + * @debugfs_mutex: + * + * Protects &debugfs_list access. + */ + struct mutex debugfs_mutex; + + /** + * @debugfs_list: + * + * List of debugfs files to be created by the DRM device. The files + * must be added during drm_dev_register(). + */ + struct list_head debugfs_list; + /* Everything below here is for legacy driver, never use! */ /* private: */ #if IS_ENABLED(CONFIG_DRM_LEGACY)