On 2/8/23 15:17, Daniel Vetter wrote:
On Wed, Feb 08, 2023 at 07:12:22PM +0100, Daniel Vetter wrote:
On Tue, Jan 31, 2023 at 04:58:25PM -0300, Maíra Canal wrote:
Currently, the drivers need to access the struct drm_debugfs_entry to
get the proper device on the show callback. There is no need for such
thing, as you can wrap the show callback in order to provide to the
driver the proper parameters: the struct seq_file, the struct drm_device
and the driver-specific data stored in the struct drm_debugfs_info.
Therefore, make the show callback pass the pointer to the right object
in the parameters, which makes the API more type-safe.
Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
---
drivers/gpu/drm/arm/hdlcd_drv.c | 8 ++------
drivers/gpu/drm/drm_atomic.c | 4 +---
drivers/gpu/drm/drm_client.c | 5 ++---
drivers/gpu/drm/drm_debugfs.c | 25 ++++++++++++-------------
drivers/gpu/drm/drm_framebuffer.c | 4 +---
drivers/gpu/drm/drm_gem_vram_helper.c | 5 ++---
drivers/gpu/drm/gud/gud_drv.c | 5 ++---
drivers/gpu/drm/v3d/v3d_debugfs.c | 16 ++++------------
drivers/gpu/drm/vc4/vc4_bo.c | 4 +---
drivers/gpu/drm/vc4/vc4_debugfs.c | 6 ++----
drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++----
drivers/gpu/drm/vc4/vc4_hvs.c | 8 ++------
drivers/gpu/drm/vc4/vc4_v3d.c | 4 +---
drivers/gpu/drm/vkms/vkms_drv.c | 4 +---
include/drm/drm_debugfs.h | 14 ++++++++------
15 files changed, 43 insertions(+), 75 deletions(-)
[...]
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index 423aa3de506a..0fb7ad5f6893 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -36,6 +36,9 @@
#include <linux/mutex.h>
#include <linux/types.h>
#include <linux/seq_file.h>
+
+struct drm_device;
+
/**
* struct drm_info_list - debugfs info list entry
*
@@ -108,11 +111,10 @@ struct drm_debugfs_info {
/**
* @show:
*
- * Show callback. &seq_file->private will be set to the &struct
- * drm_debugfs_entry corresponding to the instance of this info
- * on a given &struct drm_device.
+ * Show callback. This callback will be casted in order to provide
+ * the &seq_file, the DRM object and the data stored in this struct.
*/
- int (*show)(struct seq_file*, void*);
+ void *show;
The problem here is that with this we loose type-checking, and so all the
users of drm_debugfs_add_file() have been missed in the conversion. That's
not very good :-/
Correction, you didn't miss that, but it's the risk that could happen
because the driver doesn't check things.
I think the only way to sort this out is if we duplicate the driver-facing
functions/structs (maybe we don't need the add_files() functions in all
cases?), and only use the type-unsafe void* internally.
Since I didnt' spell it out: If you only keep the change to add the
drm_device *dev pointer in this patch, but keep the full type everywhere,
then struct drm_debugfs_entry becomes an implementation detail and you can
move it into drm_debugfs.c. Once you have that you can throw out the
driver api facing struct drm_debugfs_info and just put all the required
pointers and things directly in there as void *, and it should work out
with reasonable amounts of code sharing, while the driver api all stays
type safe.
So basically, the idea is to make this patchset using
int (*show)(struct seq_file*, struct drm_device*, void*)? I'm not sure
how could we could reuse the struct drm_debugfs_info if we don't use
void * in the show callback, as add_files needs the struct.
The type-checking is handle by the add_file and the show wrapper.
Otherwise, we would have to duplicate the drm_debugfs_info and add_file
for each object. Or do you have any other idea on how to implement
it?
Anyway, I'll send a new version with
int (*show)(struct seq_file*, struct drm_device*, void*) and moving
the struct drm_debugfs_entry to drm_debugfs.c and we can keep working
on the API.
Thanks for all the feedback!
Best Regards,
- Maíra Canal
-Daniel
-Daniel
/** @driver_features: Required driver features for this entry. */
u32 driver_features;
@@ -146,7 +148,7 @@ int drm_debugfs_remove_files(const struct drm_info_list *files,
int count, struct drm_minor *minor);
void drm_debugfs_add_file(struct drm_device *dev, const char *name,
- int (*show)(struct seq_file*, void*), void *data);
+ int (*show)(struct seq_file*, struct drm_device*, void*), void *data);
void drm_debugfs_add_files(struct drm_device *dev,
const struct drm_debugfs_info *files, int count);
@@ -163,7 +165,7 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
}
static inline void drm_debugfs_add_file(struct drm_device *dev, const char *name,
- int (*show)(struct seq_file*, void*),
+ int (*show)(struct seq_file*, struct drm_device*, void*),
void *data)
{}
--
2.39.1
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
