[PATCH v3 5/6] drm/debugfs: Make the show callback pass the pointer to the right object

[Maíra Canal]

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 at igalia.com>
>>> ---
>>>   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
>