[PATCH 00/13] drm/debugfs: Create a debugfs infrastructure for kms objects

[PATCH 00/13] drm/debugfs: Create a debugfs infrastructure for kms objects

[Maíra Canal]

This series introduces the initial structure to improve the support of kms
objects by the DRM debugfs [1].

Currently, we can create debugfs files at late-register hooks with the function
drm_debugfs_add_file() through the functionality implemented by the function
drm_debugfs_late_register(), which creates the debugfs files after the
registration of the kms objects. But, this was a stop-gap 
while we didn't have proper debugfs infrastructure for the kms objects.

Therefore, this series will add a debugfs infrastructure to the connector,
crtc, and encoder objects similar to the device-centered debugfs helpers [2].

Patches 1 and 2 will create macros to avoid copypasting the core management of
the linked list. Patches 3 through 5 will introduce the structure for the
connector, encoder, and crtc respectively. Patches 6 through 11 will convert
the old structure to the new structure for 2 drivers: vc4 and sti. Finally,
patch 12 will delete the unnecessary drm_debugfs_late_register() function.

[1] https://cgit.freedesktop.org/drm/drm/tree/Documentation/gpu/todo.rst#n506/
[2] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1c9cacbea880513a896aee65a5c58007bcb55653

Best Regards,
- Maíra Canal

Maíra Canal (13):
  drm/debugfs: Create helper to add debugfs files to device's list
  drm/debugfs: Create helper to create debugfs files from list
  drm/debugfs: Create a debugfs infrastructure for connectors
  drm/debugfs: Create a debugfs infrastructure for encoders
  drm/debugfs: Create a debugfs infrastructure for CRTC
  drm/vc4: Split variable instantiation of vc4_debugfs_regset32()
  drm/vc4: Use the encoders' debugfs infrastructure
  drm/vc4: Use the crtc's debugfs infrastructure
  drm/sti: Use the crtc's debugfs infrastructure
  drm/sti: Use the connectors' debugfs infrastructure
  drm/sti: Use the encoders' debugfs infrastructure
  drm/debugfs: Remove the debugfs late register function
  drm/todo: Update the debugfs clean up task

 Documentation/gpu/todo.rst           |  14 ++-
 drivers/gpu/drm/drm_connector.c      |   5 +
 drivers/gpu/drm/drm_crtc.c           |   5 +
 drivers/gpu/drm/drm_debugfs.c        | 141 +++++++++++++++++++++++----
 drivers/gpu/drm/drm_encoder.c        |   6 ++
 drivers/gpu/drm/drm_internal.h       |  16 ++-
 drivers/gpu/drm/drm_mode_config.c    |   2 -
 drivers/gpu/drm/sti/sti_compositor.c |   6 +-
 drivers/gpu/drm/sti/sti_compositor.h |   2 +-
 drivers/gpu/drm/sti/sti_crtc.c       |   2 +-
 drivers/gpu/drm/sti/sti_dvo.c        |  21 +---
 drivers/gpu/drm/sti/sti_hda.c        |  21 +---
 drivers/gpu/drm/sti/sti_hdmi.c       |  21 +---
 drivers/gpu/drm/sti/sti_mixer.c      |  31 +-----
 drivers/gpu/drm/sti/sti_mixer.h      |   2 +-
 drivers/gpu/drm/sti/sti_tvout.c      |  21 +---
 drivers/gpu/drm/sti/sti_vid.c        |  19 +---
 drivers/gpu/drm/sti/sti_vid.h        |   2 +-
 drivers/gpu/drm/vc4/vc4_crtc.c       |   5 +-
 drivers/gpu/drm/vc4/vc4_debugfs.c    |  55 +++++++++--
 drivers/gpu/drm/vc4/vc4_dpi.c        |   3 +-
 drivers/gpu/drm/vc4/vc4_drv.h        |  16 +++
 drivers/gpu/drm/vc4/vc4_dsi.c        |   3 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c       |   5 +-
 drivers/gpu/drm/vc4/vc4_vec.c        |   3 +-
 include/drm/drm_connector.h          |  15 +++
 include/drm/drm_crtc.h               |  15 +++
 include/drm/drm_debugfs.h            |  77 +++++++++++++++
 include/drm/drm_encoder.h            |  15 +++
 29 files changed, 384 insertions(+), 165 deletions(-)

-- 
2.39.0

[PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Maíra Canal]

Create a helper to encapsulate the code that adds a new debugfs file to
a linked list related to a object. Moreover, the helper also provides
more flexibily on the type of the object, allowing to use the helper for
other types of drm_debugfs_entry.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 4f643a490dc3..255d2068ac16 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
 	minor->debugfs_root = NULL;
 }
 
+#define drm_debugfs_add_file_to_list(component) do {			\
+		entry->file.name = name;				\
+		entry->file.show = show;				\
+		entry->file.data = data;				\
+		entry->component = (component);				\
+									\
+		mutex_lock(&(component)->debugfs_mutex);		\
+		list_add(&entry->list, &(component)->debugfs_list);	\
+		mutex_unlock(&(component)->debugfs_mutex);		\
+	} while (0)
+
 /**
  * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
  * @dev: drm device for the ioctl
@@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
 	if (!entry)
 		return;
 
-	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);
+	drm_debugfs_add_file_to_list(dev);
 }
 EXPORT_SYMBOL(drm_debugfs_add_file);
 
-- 
2.39.0

Re: [PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Jani Nikula]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Create a helper to encapsulate the code that adds a new debugfs file to
> a linked list related to a object. Moreover, the helper also provides
> more flexibily on the type of the object, allowing to use the helper for
> other types of drm_debugfs_entry.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 4f643a490dc3..255d2068ac16 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
>  	minor->debugfs_root = NULL;
>  }
>  
> +#define drm_debugfs_add_file_to_list(component) do {			\
> +		entry->file.name = name;				\
> +		entry->file.show = show;				\
> +		entry->file.data = data;				\
> +		entry->component = (component);				\
> +									\
> +		mutex_lock(&(component)->debugfs_mutex);		\
> +		list_add(&entry->list, &(component)->debugfs_list);	\
> +		mutex_unlock(&(component)->debugfs_mutex);		\
> +	} while (0)

In general, please don't add macros that implicitly depend on certain
local variable names. In this case, "entry".

But I'm also not convinced about the usefulness of adding this kind of
"generics". Sure, it'll save you a few lines here and there, but I think
overall it's just confusing more than it's useful.


BR,
Jani.

> +
>  /**
>   * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
>   * @dev: drm device for the ioctl
> @@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>  	if (!entry)
>  		return;
>  
> -	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);
> +	drm_debugfs_add_file_to_list(dev);
>  }
>  EXPORT_SYMBOL(drm_debugfs_add_file);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Daniel Vetter]

On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
> On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> > Create a helper to encapsulate the code that adds a new debugfs file to
> > a linked list related to a object. Moreover, the helper also provides
> > more flexibily on the type of the object, allowing to use the helper for
> > other types of drm_debugfs_entry.
> >
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > ---
> >  drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index 4f643a490dc3..255d2068ac16 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
> >  	minor->debugfs_root = NULL;
> >  }
> >  
> > +#define drm_debugfs_add_file_to_list(component) do {			\
> > +		entry->file.name = name;				\
> > +		entry->file.show = show;				\
> > +		entry->file.data = data;				\
> > +		entry->component = (component);				\
> > +									\
> > +		mutex_lock(&(component)->debugfs_mutex);		\
> > +		list_add(&entry->list, &(component)->debugfs_list);	\
> > +		mutex_unlock(&(component)->debugfs_mutex);		\
> > +	} while (0)
> 
> In general, please don't add macros that implicitly depend on certain
> local variable names. In this case, "entry".
> 
> But I'm also not convinced about the usefulness of adding this kind of
> "generics". Sure, it'll save you a few lines here and there, but I think
> overall it's just confusing more than it's useful.

So the non-generics way I guess would be to
- pass the right pointer to the functions as an explicit parameter (struct
  drm_device|crtc|connector *, )
- make drm_debugfs_entry and implementation detail
- switch the pointer in there to void *, have glue show functions for each
  case which do nothing else than cast from void * to the right type
  (both for the parameter and the function pointer)
- have a single function which takes that void *entry list and a pointer
  to the debugfs director to add them all for code sharing

I think this should work for ->show, but for ->fops it becomes a rather
big mess I fear. Maybe for ->fops (and also for ->show for now) we leave
the explicit parameter out and just rely on seq_file->private or whatever
it was.

Or just copypaste, it's not that much code really :-)
-Daniel

> 
> 
> BR,
> Jani.
> 
> > +
> >  /**
> >   * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
> >   * @dev: drm device for the ioctl
> > @@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
> >  	if (!entry)
> >  		return;
> >  
> > -	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);
> > +	drm_debugfs_add_file_to_list(dev);
> >  }
> >  EXPORT_SYMBOL(drm_debugfs_add_file);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Jani Nikula]

On Thu, 12 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
>> On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
>> > Create a helper to encapsulate the code that adds a new debugfs file to
>> > a linked list related to a object. Moreover, the helper also provides
>> > more flexibily on the type of the object, allowing to use the helper for
>> > other types of drm_debugfs_entry.
>> >
>> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> > ---
>> >  drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
>> >  1 file changed, 12 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> > index 4f643a490dc3..255d2068ac16 100644
>> > --- a/drivers/gpu/drm/drm_debugfs.c
>> > +++ b/drivers/gpu/drm/drm_debugfs.c
>> > @@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
>> >  	minor->debugfs_root = NULL;
>> >  }
>> >  
>> > +#define drm_debugfs_add_file_to_list(component) do {			\
>> > +		entry->file.name = name;				\
>> > +		entry->file.show = show;				\
>> > +		entry->file.data = data;				\
>> > +		entry->component = (component);				\
>> > +									\
>> > +		mutex_lock(&(component)->debugfs_mutex);		\
>> > +		list_add(&entry->list, &(component)->debugfs_list);	\
>> > +		mutex_unlock(&(component)->debugfs_mutex);		\
>> > +	} while (0)
>> 
>> In general, please don't add macros that implicitly depend on certain
>> local variable names. In this case, "entry".
>> 
>> But I'm also not convinced about the usefulness of adding this kind of
>> "generics". Sure, it'll save you a few lines here and there, but I think
>> overall it's just confusing more than it's useful.
>
> So the non-generics way I guess would be to
> - pass the right pointer to the functions as an explicit parameter (struct
>   drm_device|crtc|connector *, )
> - make drm_debugfs_entry and implementation detail
> - switch the pointer in there to void *, have glue show functions for each
>   case which do nothing else than cast from void * to the right type
>   (both for the parameter and the function pointer)
> - have a single function which takes that void *entry list and a pointer
>   to the debugfs director to add them all for code sharing
>
> I think this should work for ->show, but for ->fops it becomes a rather
> big mess I fear. Maybe for ->fops (and also for ->show for now) we leave
> the explicit parameter out and just rely on seq_file->private or whatever
> it was.

Similar ideas in https://lore.kernel.org/r/878ri8glee.fsf@intel.com

I think for the more general ->fops case, the question really becomes
does drm need *all* the functionality of ->fops, or can we get away with
providing just show/write functions pointers, and wrapping it all inside
drm_debugfs.c? The question *now* is avoiding to paint ourselves in the
corner and requiring a ton of *extra* work to make that happen.


BR,
Jani.




>
> Or just copypaste, it's not that much code really :-)
> -Daniel
>
>> 
>> 
>> BR,
>> Jani.
>> 
>> > +
>> >  /**
>> >   * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
>> >   * @dev: drm device for the ioctl
>> > @@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>> >  	if (!entry)
>> >  		return;
>> >  
>> > -	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);
>> > +	drm_debugfs_add_file_to_list(dev);
>> >  }
>> >  EXPORT_SYMBOL(drm_debugfs_add_file);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH 02/13] drm/debugfs: Create helper to create debugfs files from list

[Maíra Canal]

Create a helper to encapsulate the code that creates a new debugfs file
from a linked list related to an object. Moreover, the helper also provides
more flexibily on the type of the object.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_debugfs.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 255d2068ac16..23f6ed7b5d68 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -218,6 +218,16 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
 }
 EXPORT_SYMBOL(drm_debugfs_create_files);
 
+#define drm_create_file_from_list(component) do {					\
+		list_for_each_entry_safe(entry, tmp, &(component)->debugfs_list,	\
+					 list) {					\
+			debugfs_create_file(entry->file.name, 0444,			\
+					    minor->debugfs_root, entry,			\
+					    &drm_debugfs_entry_fops);			\
+			list_del(&entry->list);						\
+		}									\
+	} while (0)
+
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		     struct dentry *root)
 {
@@ -245,11 +255,7 @@ 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_safe(entry, tmp, &dev->debugfs_list, list) {
-		debugfs_create_file(entry->file.name, 0444,
-				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
-		list_del(&entry->list);
-	}
+	drm_create_file_from_list(dev);
 
 	return 0;
 }
@@ -262,11 +268,7 @@ void drm_debugfs_late_register(struct drm_device *dev)
 	if (!minor)
 		return;
 
-	list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
-		debugfs_create_file(entry->file.name, 0444,
-				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
-		list_del(&entry->list);
-	}
+	drm_create_file_from_list(dev);
 }
 
 int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
-- 
2.39.0

Re: [PATCH 02/13] drm/debugfs: Create helper to create debugfs files from list

[Jani Nikula]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Create a helper to encapsulate the code that creates a new debugfs file
> from a linked list related to an object. Moreover, the helper also provides
> more flexibily on the type of the object.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 255d2068ac16..23f6ed7b5d68 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -218,6 +218,16 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>  
> +#define drm_create_file_from_list(component) do {					\
> +		list_for_each_entry_safe(entry, tmp, &(component)->debugfs_list,	\
> +					 list) {					\
> +			debugfs_create_file(entry->file.name, 0444,			\
> +					    minor->debugfs_root, entry,			\
> +					    &drm_debugfs_entry_fops);			\
> +			list_del(&entry->list);						\
> +		}									\
> +	} while (0)
> +

Same here as in the previous patch. I really think you should try to
figure out how to break this into useful functions, and avoid macros
like this.

BR,
Jani.

>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root)
>  {
> @@ -245,11 +255,7 @@ 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_safe(entry, tmp, &dev->debugfs_list, list) {
> -		debugfs_create_file(entry->file.name, 0444,
> -				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> -		list_del(&entry->list);
> -	}
> +	drm_create_file_from_list(dev);
>  
>  	return 0;
>  }
> @@ -262,11 +268,7 @@ void drm_debugfs_late_register(struct drm_device *dev)
>  	if (!minor)
>  		return;
>  
> -	list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> -		debugfs_create_file(entry->file.name, 0444,
> -				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> -		list_del(&entry->list);
> -	}
> +	drm_create_file_from_list(dev);
>  }
>  
>  int drm_debugfs_remove_files(const struct drm_info_list *files, int count,

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH 03/13] drm/debugfs: Create a debugfs infrastructure for connectors

[Maíra Canal]

Introduce the ability to add DRM debugfs files to a list managed by the
connector and, during drm_connector_register(), all added files will be
created at once.

Moreover, introduce some typesafety as struct drm_debugfs_connector_entry
holds a drm_connector instead of a drm_device. So, the drivers can get
a connector object directly from the struct drm_debugfs_connector_entry
in the show() callback.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_connector.c |  5 +++++
 drivers/gpu/drm/drm_debugfs.c   | 35 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_internal.h  |  5 +++++
 include/drm/drm_connector.h     | 15 ++++++++++++++
 include/drm/drm_debugfs.h       | 26 ++++++++++++++++++++++++
 5 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 8d92777e57dd..c93655bb0edf 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -273,8 +273,10 @@ static int __drm_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->global_connector_list_entry);
 	INIT_LIST_HEAD(&connector->probed_modes);
 	INIT_LIST_HEAD(&connector->modes);
+	INIT_LIST_HEAD(&connector->debugfs_list);
 	mutex_init(&connector->mutex);
 	mutex_init(&connector->edid_override_mutex);
+	mutex_init(&connector->debugfs_mutex);
 	connector->edid_blob_ptr = NULL;
 	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
@@ -581,6 +583,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 						       connector->state);
 
 	mutex_destroy(&connector->mutex);
+	mutex_destroy(&connector->debugfs_mutex);
 
 	memset(connector, 0, sizeof(*connector));
 
@@ -627,6 +630,8 @@ int drm_connector_register(struct drm_connector *connector)
 			goto err_debugfs;
 	}
 
+	drm_debugfs_connector_init(connector);
+
 	drm_mode_object_register(connector->dev, &connector->base);
 
 	connector->registration_state = DRM_CONNECTOR_REGISTERED;
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 23f6ed7b5d68..d9ec1ed5a7ec 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -260,6 +260,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	return 0;
 }
 
+void drm_debugfs_connector_init(struct drm_connector *connector)
+{
+	struct drm_minor *minor = connector->dev->primary;
+	struct drm_debugfs_connector_entry *entry, *tmp;
+
+	if (!minor)
+		return;
+
+	drm_create_file_from_list(connector);
+}
+
 void drm_debugfs_late_register(struct drm_device *dev)
 {
 	struct drm_minor *minor = dev->primary;
@@ -369,6 +380,30 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
 }
 EXPORT_SYMBOL(drm_debugfs_add_files);
 
+/**
+ * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
+ * @connector: DRM connector object
+ * @name: debugfs file name
+ * @show: show callback
+ * @data: driver-private data, should not be device-specific
+ *
+ * Add a given file entry to the DRM connector debugfs file list to be created on
+ * drm_debugfs_connector_init().
+ */
+void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
+				    int (*show)(struct seq_file*, void*), void *data)
+{
+	struct drm_debugfs_connector_entry *entry = drmm_kzalloc(connector->dev,
+								 sizeof(*entry),
+								 GFP_KERNEL);
+
+	if (!entry)
+		return;
+
+	drm_debugfs_add_file_to_list(connector);
+}
+EXPORT_SYMBOL(drm_debugfs_connector_add_file);
+
 static int connector_show(struct seq_file *m, void *data)
 {
 	struct drm_connector *connector = m->private;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ed2103ee272c..dd9d7b8b45bd 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
 #if defined(CONFIG_DEBUG_FS)
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		     struct dentry *root);
+void drm_debugfs_connector_init(struct drm_connector *connector);
 void drm_debugfs_cleanup(struct drm_minor *minor);
 void drm_debugfs_late_register(struct drm_device *dev);
 void drm_debugfs_connector_add(struct drm_connector *connector);
@@ -199,6 +200,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	return 0;
 }
 
+static inline void drm_debugfs_connector_init(struct drm_connector *connector)
+{
+}
+
 static inline void drm_debugfs_cleanup(struct drm_minor *minor)
 {
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9037f1317aee..51340f3162ed 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1720,6 +1720,21 @@ struct drm_connector {
 	/** @debugfs_entry: debugfs directory for this connector */
 	struct dentry *debugfs_entry;
 
+	/**
+	 * @debugfs_mutex:
+	 *
+	 * Protects &debugfs_list access.
+	 */
+	struct mutex debugfs_mutex;
+
+	/**
+	 * @debugfs_list:
+	 *
+	 * List of debugfs files to be created by the DRM connector. The files
+	 * must be added during drm_connector_register().
+	 */
+	struct list_head debugfs_list;
+
 	/**
 	 * @state:
 	 *
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index 7616f457ce70..c09c82274622 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -122,6 +122,23 @@ struct drm_debugfs_entry {
 	struct list_head list;
 };
 
+/**
+ * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
+ *
+ * This structure represents a debugfs file, as an instantiation of a &struct
+ * drm_debugfs_info on a &struct drm_connector.
+ */
+struct drm_debugfs_connector_entry {
+	/** @connector: &struct drm_connector for this node. */
+	struct drm_connector *connector;
+
+	/** @file: Template for this node. */
+	struct drm_debugfs_info file;
+
+	/** @list: Linked list of all connector nodes. */
+	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,
@@ -134,6 +151,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
 
 void drm_debugfs_add_files(struct drm_device *dev,
 			   const struct drm_debugfs_info *files, int count);
+
+void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
+				    int (*show)(struct seq_file*, void*), void *data);
 #else
 static inline void drm_debugfs_create_files(const struct drm_info_list *files,
 					    int count, struct dentry *root,
@@ -155,6 +175,12 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
 					 const struct drm_debugfs_info *files,
 					 int count)
 {}
+
+static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
+						  const char *name,
+						  int (*show)(struct seq_file*, void*),
+						  void *data)
+{}
 #endif
 
 #endif /* _DRM_DEBUGFS_H_ */
-- 
2.39.0

Re: [PATCH 03/13] drm/debugfs: Create a debugfs infrastructure for connectors

[Jani Nikula]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Introduce the ability to add DRM debugfs files to a list managed by the
> connector and, during drm_connector_register(), all added files will be
> created at once.
>
> Moreover, introduce some typesafety as struct drm_debugfs_connector_entry
> holds a drm_connector instead of a drm_device. So, the drivers can get
> a connector object directly from the struct drm_debugfs_connector_entry
> in the show() callback.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  5 +++++
>  drivers/gpu/drm/drm_debugfs.c   | 35 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_internal.h  |  5 +++++
>  include/drm/drm_connector.h     | 15 ++++++++++++++
>  include/drm/drm_debugfs.h       | 26 ++++++++++++++++++++++++
>  5 files changed, 86 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8d92777e57dd..c93655bb0edf 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -273,8 +273,10 @@ static int __drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->global_connector_list_entry);
>  	INIT_LIST_HEAD(&connector->probed_modes);
>  	INIT_LIST_HEAD(&connector->modes);
> +	INIT_LIST_HEAD(&connector->debugfs_list);
>  	mutex_init(&connector->mutex);
>  	mutex_init(&connector->edid_override_mutex);
> +	mutex_init(&connector->debugfs_mutex);

In another mail, I suggested adding a struct wrapper for debugfs_list
and debugfs_mutex. I think those should also be initialized by debugfs
code.

The initializer would not have to be connector/encoder/crtc specific, it
could be:

	drm_debugfs_something_init(&connector->debugfs_something);

>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> @@ -581,6 +583,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  						       connector->state);
>  
>  	mutex_destroy(&connector->mutex);
> +	mutex_destroy(&connector->debugfs_mutex);

Ditto for cleanup.

>  
>  	memset(connector, 0, sizeof(*connector));
>  
> @@ -627,6 +630,8 @@ int drm_connector_register(struct drm_connector *connector)
>  			goto err_debugfs;
>  	}
>  
> +	drm_debugfs_connector_init(connector);

Just perhaps this should be called _register()? The name gives a strong
feeling at which stage it is called, and the init here feels like it
should be moved earlier, to connector init, instead of connector
register.

> +
>  	drm_mode_object_register(connector->dev, &connector->base);
>  
>  	connector->registration_state = DRM_CONNECTOR_REGISTERED;
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 23f6ed7b5d68..d9ec1ed5a7ec 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -260,6 +260,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	return 0;
>  }
>  
> +void drm_debugfs_connector_init(struct drm_connector *connector)
> +{
> +	struct drm_minor *minor = connector->dev->primary;
> +	struct drm_debugfs_connector_entry *entry, *tmp;
> +
> +	if (!minor)
> +		return;
> +
> +	drm_create_file_from_list(connector);
> +}
> +
>  void drm_debugfs_late_register(struct drm_device *dev)
>  {
>  	struct drm_minor *minor = dev->primary;
> @@ -369,6 +380,30 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
>  }
>  EXPORT_SYMBOL(drm_debugfs_add_files);
>  
> +/**
> + * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
> + * @connector: DRM connector object
> + * @name: debugfs file name
> + * @show: show callback
> + * @data: driver-private data, should not be device-specific
> + *
> + * Add a given file entry to the DRM connector debugfs file list to be created on
> + * drm_debugfs_connector_init().
> + */
> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
> +				    int (*show)(struct seq_file*, void*), void *data)
> +{
> +	struct drm_debugfs_connector_entry *entry = drmm_kzalloc(connector->dev,
> +								 sizeof(*entry),
> +								 GFP_KERNEL);
> +
> +	if (!entry)
> +		return;
> +
> +	drm_debugfs_add_file_to_list(connector);
> +}
> +EXPORT_SYMBOL(drm_debugfs_connector_add_file);
> +
>  static int connector_show(struct seq_file *m, void *data)
>  {
>  	struct drm_connector *connector = m->private;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ed2103ee272c..dd9d7b8b45bd 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
>  #if defined(CONFIG_DEBUG_FS)
>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root);
> +void drm_debugfs_connector_init(struct drm_connector *connector);
>  void drm_debugfs_cleanup(struct drm_minor *minor);
>  void drm_debugfs_late_register(struct drm_device *dev);
>  void drm_debugfs_connector_add(struct drm_connector *connector);
> @@ -199,6 +200,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	return 0;
>  }
>  
> +static inline void drm_debugfs_connector_init(struct drm_connector *connector)
> +{
> +}
> +
>  static inline void drm_debugfs_cleanup(struct drm_minor *minor)
>  {
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9037f1317aee..51340f3162ed 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1720,6 +1720,21 @@ struct drm_connector {
>  	/** @debugfs_entry: debugfs directory for this connector */
>  	struct dentry *debugfs_entry;
>  
> +	/**
> +	 * @debugfs_mutex:
> +	 *
> +	 * Protects &debugfs_list access.
> +	 */
> +	struct mutex debugfs_mutex;
> +
> +	/**
> +	 * @debugfs_list:
> +	 *
> +	 * List of debugfs files to be created by the DRM connector. The files
> +	 * must be added during drm_connector_register().
> +	 */
> +	struct list_head debugfs_list;
> +
>  	/**
>  	 * @state:
>  	 *
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index 7616f457ce70..c09c82274622 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -122,6 +122,23 @@ struct drm_debugfs_entry {
>  	struct list_head list;
>  };
>  
> +/**
> + * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
> + *
> + * This structure represents a debugfs file, as an instantiation of a &struct
> + * drm_debugfs_info on a &struct drm_connector.
> + */
> +struct drm_debugfs_connector_entry {
> +	/** @connector: &struct drm_connector for this node. */
> +	struct drm_connector *connector;
> +
> +	/** @file: Template for this node. */
> +	struct drm_debugfs_info file;
> +
> +	/** @list: Linked list of all connector nodes. */
> +	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,
> @@ -134,6 +151,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>  
>  void drm_debugfs_add_files(struct drm_device *dev,
>  			   const struct drm_debugfs_info *files, int count);
> +
> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
> +				    int (*show)(struct seq_file*, void*), void *data);
>  #else
>  static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>  					    int count, struct dentry *root,
> @@ -155,6 +175,12 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
>  					 const struct drm_debugfs_info *files,
>  					 int count)
>  {}
> +
> +static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
> +						  const char *name,
> +						  int (*show)(struct seq_file*, void*),
> +						  void *data)
> +{}
>  #endif
>  
>  #endif /* _DRM_DEBUGFS_H_ */

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 03/13] drm/debugfs: Create a debugfs infrastructure for connectors

[Daniel Vetter]

On Wed, Jan 11, 2023 at 02:37:38PM -0300, Maíra Canal wrote:
> Introduce the ability to add DRM debugfs files to a list managed by the
> connector and, during drm_connector_register(), all added files will be
> created at once.
> 
> Moreover, introduce some typesafety as struct drm_debugfs_connector_entry
> holds a drm_connector instead of a drm_device. So, the drivers can get
> a connector object directly from the struct drm_debugfs_connector_entry
> in the show() callback.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  5 +++++
>  drivers/gpu/drm/drm_debugfs.c   | 35 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_internal.h  |  5 +++++
>  include/drm/drm_connector.h     | 15 ++++++++++++++
>  include/drm/drm_debugfs.h       | 26 ++++++++++++++++++++++++
>  5 files changed, 86 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8d92777e57dd..c93655bb0edf 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -273,8 +273,10 @@ static int __drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->global_connector_list_entry);
>  	INIT_LIST_HEAD(&connector->probed_modes);
>  	INIT_LIST_HEAD(&connector->modes);
> +	INIT_LIST_HEAD(&connector->debugfs_list);
>  	mutex_init(&connector->mutex);
>  	mutex_init(&connector->edid_override_mutex);
> +	mutex_init(&connector->debugfs_mutex);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> @@ -581,6 +583,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  						       connector->state);
>  
>  	mutex_destroy(&connector->mutex);
> +	mutex_destroy(&connector->debugfs_mutex);
>  
>  	memset(connector, 0, sizeof(*connector));
>  
> @@ -627,6 +630,8 @@ int drm_connector_register(struct drm_connector *connector)
>  			goto err_debugfs;
>  	}
>  
> +	drm_debugfs_connector_init(connector);
> +
>  	drm_mode_object_register(connector->dev, &connector->base);
>  
>  	connector->registration_state = DRM_CONNECTOR_REGISTERED;
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 23f6ed7b5d68..d9ec1ed5a7ec 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -260,6 +260,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	return 0;
>  }
>  
> +void drm_debugfs_connector_init(struct drm_connector *connector)
> +{
> +	struct drm_minor *minor = connector->dev->primary;
> +	struct drm_debugfs_connector_entry *entry, *tmp;
> +
> +	if (!minor)
> +		return;
> +
> +	drm_create_file_from_list(connector);
> +}
> +
>  void drm_debugfs_late_register(struct drm_device *dev)
>  {
>  	struct drm_minor *minor = dev->primary;
> @@ -369,6 +380,30 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
>  }
>  EXPORT_SYMBOL(drm_debugfs_add_files);
>  
> +/**
> + * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
> + * @connector: DRM connector object
> + * @name: debugfs file name
> + * @show: show callback
> + * @data: driver-private data, should not be device-specific
> + *
> + * Add a given file entry to the DRM connector debugfs file list to be created on
> + * drm_debugfs_connector_init().
> + */
> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
> +				    int (*show)(struct seq_file*, void*), void *data)
> +{
> +	struct drm_debugfs_connector_entry *entry = drmm_kzalloc(connector->dev,
> +								 sizeof(*entry),
> +								 GFP_KERNEL);
> +
> +	if (!entry)
> +		return;
> +
> +	drm_debugfs_add_file_to_list(connector);
> +}
> +EXPORT_SYMBOL(drm_debugfs_connector_add_file);
> +
>  static int connector_show(struct seq_file *m, void *data)
>  {
>  	struct drm_connector *connector = m->private;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ed2103ee272c..dd9d7b8b45bd 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
>  #if defined(CONFIG_DEBUG_FS)
>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root);
> +void drm_debugfs_connector_init(struct drm_connector *connector);
>  void drm_debugfs_cleanup(struct drm_minor *minor);
>  void drm_debugfs_late_register(struct drm_device *dev);
>  void drm_debugfs_connector_add(struct drm_connector *connector);
> @@ -199,6 +200,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	return 0;
>  }
>  
> +static inline void drm_debugfs_connector_init(struct drm_connector *connector)
> +{
> +}
> +
>  static inline void drm_debugfs_cleanup(struct drm_minor *minor)
>  {
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9037f1317aee..51340f3162ed 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1720,6 +1720,21 @@ struct drm_connector {
>  	/** @debugfs_entry: debugfs directory for this connector */
>  	struct dentry *debugfs_entry;
>  
> +	/**
> +	 * @debugfs_mutex:
> +	 *
> +	 * Protects &debugfs_list access.
> +	 */
> +	struct mutex debugfs_mutex;
> +
> +	/**
> +	 * @debugfs_list:
> +	 *
> +	 * List of debugfs files to be created by the DRM connector. The files
> +	 * must be added during drm_connector_register().
> +	 */
> +	struct list_head debugfs_list;
> +
>  	/**
>  	 * @state:
>  	 *
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index 7616f457ce70..c09c82274622 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -122,6 +122,23 @@ struct drm_debugfs_entry {
>  	struct list_head list;
>  };
>  
> +/**
> + * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
> + *
> + * This structure represents a debugfs file, as an instantiation of a &struct
> + * drm_debugfs_info on a &struct drm_connector.
> + */
> +struct drm_debugfs_connector_entry {
> +	/** @connector: &struct drm_connector for this node. */
> +	struct drm_connector *connector;
> +
> +	/** @file: Template for this node. */
> +	struct drm_debugfs_info file;
> +
> +	/** @list: Linked list of all connector nodes. */
> +	struct list_head list;
> +};

I missed it in the main api, but I'm not a big fan of exposing this struct
to driver. And I don't see the need ... if we just directly put the
drm_connector into seq_file->private (or an explicit parameter for our
show function, with some glue provided) then drivers don't need to be able
to see this? This really should be just an implementation detail I think.
-Daniel

> +
>  #if defined(CONFIG_DEBUG_FS)
>  void drm_debugfs_create_files(const struct drm_info_list *files,
>  			      int count, struct dentry *root,
> @@ -134,6 +151,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>  
>  void drm_debugfs_add_files(struct drm_device *dev,
>  			   const struct drm_debugfs_info *files, int count);
> +
> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
> +				    int (*show)(struct seq_file*, void*), void *data);
>  #else
>  static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>  					    int count, struct dentry *root,
> @@ -155,6 +175,12 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
>  					 const struct drm_debugfs_info *files,
>  					 int count)
>  {}
> +
> +static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
> +						  const char *name,
> +						  int (*show)(struct seq_file*, void*),
> +						  void *data)
> +{}
>  #endif
>  
>  #endif /* _DRM_DEBUGFS_H_ */
> -- 
> 2.39.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 03/13] drm/debugfs: Create a debugfs infrastructure for connectors

[Jani Nikula]

On Thu, 12 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 11, 2023 at 02:37:38PM -0300, Maíra Canal wrote:
>> Introduce the ability to add DRM debugfs files to a list managed by the
>> connector and, during drm_connector_register(), all added files will be
>> created at once.
>> 
>> Moreover, introduce some typesafety as struct drm_debugfs_connector_entry
>> holds a drm_connector instead of a drm_device. So, the drivers can get
>> a connector object directly from the struct drm_debugfs_connector_entry
>> in the show() callback.
>> 
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c |  5 +++++
>>  drivers/gpu/drm/drm_debugfs.c   | 35 +++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_internal.h  |  5 +++++
>>  include/drm/drm_connector.h     | 15 ++++++++++++++
>>  include/drm/drm_debugfs.h       | 26 ++++++++++++++++++++++++
>>  5 files changed, 86 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 8d92777e57dd..c93655bb0edf 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -273,8 +273,10 @@ static int __drm_connector_init(struct drm_device *dev,
>>  	INIT_LIST_HEAD(&connector->global_connector_list_entry);
>>  	INIT_LIST_HEAD(&connector->probed_modes);
>>  	INIT_LIST_HEAD(&connector->modes);
>> +	INIT_LIST_HEAD(&connector->debugfs_list);
>>  	mutex_init(&connector->mutex);
>>  	mutex_init(&connector->edid_override_mutex);
>> +	mutex_init(&connector->debugfs_mutex);
>>  	connector->edid_blob_ptr = NULL;
>>  	connector->epoch_counter = 0;
>>  	connector->tile_blob_ptr = NULL;
>> @@ -581,6 +583,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>>  						       connector->state);
>>  
>>  	mutex_destroy(&connector->mutex);
>> +	mutex_destroy(&connector->debugfs_mutex);
>>  
>>  	memset(connector, 0, sizeof(*connector));
>>  
>> @@ -627,6 +630,8 @@ int drm_connector_register(struct drm_connector *connector)
>>  			goto err_debugfs;
>>  	}
>>  
>> +	drm_debugfs_connector_init(connector);
>> +
>>  	drm_mode_object_register(connector->dev, &connector->base);
>>  
>>  	connector->registration_state = DRM_CONNECTOR_REGISTERED;
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 23f6ed7b5d68..d9ec1ed5a7ec 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -260,6 +260,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>>  	return 0;
>>  }
>>  
>> +void drm_debugfs_connector_init(struct drm_connector *connector)
>> +{
>> +	struct drm_minor *minor = connector->dev->primary;
>> +	struct drm_debugfs_connector_entry *entry, *tmp;
>> +
>> +	if (!minor)
>> +		return;
>> +
>> +	drm_create_file_from_list(connector);
>> +}
>> +
>>  void drm_debugfs_late_register(struct drm_device *dev)
>>  {
>>  	struct drm_minor *minor = dev->primary;
>> @@ -369,6 +380,30 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
>>  }
>>  EXPORT_SYMBOL(drm_debugfs_add_files);
>>  
>> +/**
>> + * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
>> + * @connector: DRM connector object
>> + * @name: debugfs file name
>> + * @show: show callback
>> + * @data: driver-private data, should not be device-specific
>> + *
>> + * Add a given file entry to the DRM connector debugfs file list to be created on
>> + * drm_debugfs_connector_init().
>> + */
>> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
>> +				    int (*show)(struct seq_file*, void*), void *data)
>> +{
>> +	struct drm_debugfs_connector_entry *entry = drmm_kzalloc(connector->dev,
>> +								 sizeof(*entry),
>> +								 GFP_KERNEL);
>> +
>> +	if (!entry)
>> +		return;
>> +
>> +	drm_debugfs_add_file_to_list(connector);
>> +}
>> +EXPORT_SYMBOL(drm_debugfs_connector_add_file);
>> +
>>  static int connector_show(struct seq_file *m, void *data)
>>  {
>>  	struct drm_connector *connector = m->private;
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index ed2103ee272c..dd9d7b8b45bd 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
>>  #if defined(CONFIG_DEBUG_FS)
>>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>>  		     struct dentry *root);
>> +void drm_debugfs_connector_init(struct drm_connector *connector);
>>  void drm_debugfs_cleanup(struct drm_minor *minor);
>>  void drm_debugfs_late_register(struct drm_device *dev);
>>  void drm_debugfs_connector_add(struct drm_connector *connector);
>> @@ -199,6 +200,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>>  	return 0;
>>  }
>>  
>> +static inline void drm_debugfs_connector_init(struct drm_connector *connector)
>> +{
>> +}
>> +
>>  static inline void drm_debugfs_cleanup(struct drm_minor *minor)
>>  {
>>  }
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 9037f1317aee..51340f3162ed 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1720,6 +1720,21 @@ struct drm_connector {
>>  	/** @debugfs_entry: debugfs directory for this connector */
>>  	struct dentry *debugfs_entry;
>>  
>> +	/**
>> +	 * @debugfs_mutex:
>> +	 *
>> +	 * Protects &debugfs_list access.
>> +	 */
>> +	struct mutex debugfs_mutex;
>> +
>> +	/**
>> +	 * @debugfs_list:
>> +	 *
>> +	 * List of debugfs files to be created by the DRM connector. The files
>> +	 * must be added during drm_connector_register().
>> +	 */
>> +	struct list_head debugfs_list;
>> +
>>  	/**
>>  	 * @state:
>>  	 *
>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>> index 7616f457ce70..c09c82274622 100644
>> --- a/include/drm/drm_debugfs.h
>> +++ b/include/drm/drm_debugfs.h
>> @@ -122,6 +122,23 @@ struct drm_debugfs_entry {
>>  	struct list_head list;
>>  };
>>  
>> +/**
>> + * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
>> + *
>> + * This structure represents a debugfs file, as an instantiation of a &struct
>> + * drm_debugfs_info on a &struct drm_connector.
>> + */
>> +struct drm_debugfs_connector_entry {
>> +	/** @connector: &struct drm_connector for this node. */
>> +	struct drm_connector *connector;
>> +
>> +	/** @file: Template for this node. */
>> +	struct drm_debugfs_info file;
>> +
>> +	/** @list: Linked list of all connector nodes. */
>> +	struct list_head list;
>> +};
>
> I missed it in the main api, but I'm not a big fan of exposing this struct
> to driver. And I don't see the need ... if we just directly put the
> drm_connector into seq_file->private (or an explicit parameter for our
> show function, with some glue provided) then drivers don't need to be able
> to see this? This really should be just an implementation detail I think.
> -Daniel

See also https://lore.kernel.org/r/878ri8glee.fsf@intel.com

>
>> +
>>  #if defined(CONFIG_DEBUG_FS)
>>  void drm_debugfs_create_files(const struct drm_info_list *files,
>>  			      int count, struct dentry *root,
>> @@ -134,6 +151,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>>  
>>  void drm_debugfs_add_files(struct drm_device *dev,
>>  			   const struct drm_debugfs_info *files, int count);
>> +
>> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
>> +				    int (*show)(struct seq_file*, void*), void *data);
>>  #else
>>  static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>>  					    int count, struct dentry *root,
>> @@ -155,6 +175,12 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
>>  					 const struct drm_debugfs_info *files,
>>  					 int count)
>>  {}
>> +
>> +static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
>> +						  const char *name,
>> +						  int (*show)(struct seq_file*, void*),
>> +						  void *data)
>> +{}
>>  #endif
>>  
>>  #endif /* _DRM_DEBUGFS_H_ */
>> -- 
>> 2.39.0
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH 04/13] drm/debugfs: Create a debugfs infrastructure for encoders

[Maíra Canal]

Introduce the ability to add DRM debugfs files to a list managed by the
encoder and, during drm_encoder_register_all(), all added files will be
created at once.

Moreover, introduce some typesafety as struct drm_debugfs_encoder_entry
holds a drm_encoder instead of a drm_device. So, the drivers can get
a encoder object directly from the struct drm_debugfs_encoder_entry
in the show() callback.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_debugfs.c  | 36 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_encoder.c  |  6 ++++++
 drivers/gpu/drm/drm_internal.h |  5 +++++
 include/drm/drm_debugfs.h      | 26 ++++++++++++++++++++++++
 include/drm/drm_encoder.h      | 15 ++++++++++++++
 5 files changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index d9ec1ed5a7ec..6a763fe1b031 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -36,6 +36,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_managed.h>
@@ -271,6 +272,17 @@ void drm_debugfs_connector_init(struct drm_connector *connector)
 	drm_create_file_from_list(connector);
 }
 
+void drm_debugfs_encoder_init(struct drm_encoder *encoder)
+{
+	struct drm_minor *minor = encoder->dev->primary;
+	struct drm_debugfs_encoder_entry *entry, *tmp;
+
+	if (!minor)
+		return;
+
+	drm_create_file_from_list(encoder);
+}
+
 void drm_debugfs_late_register(struct drm_device *dev)
 {
 	struct drm_minor *minor = dev->primary;
@@ -404,6 +416,30 @@ void drm_debugfs_connector_add_file(struct drm_connector *connector, const char
 }
 EXPORT_SYMBOL(drm_debugfs_connector_add_file);
 
+/**
+ * drm_debugfs_encoder_add_file - Add a given file to the DRM encoder debugfs file list
+ * @encoder: DRM encoder object
+ * @name: debugfs file name
+ * @show: show callback
+ * @data: driver-private data, should not be device-specific
+ *
+ * Add a given file entry to the DRM encoder debugfs file list to be created on
+ * drm_encoder_register_all().
+ */
+void drm_debugfs_encoder_add_file(struct drm_encoder *encoder, const char *name,
+				  int (*show)(struct seq_file*, void*), void *data)
+{
+	struct drm_debugfs_encoder_entry *entry = drmm_kzalloc(encoder->dev,
+							       sizeof(*entry),
+							       GFP_KERNEL);
+
+	if (!entry)
+		return;
+
+	drm_debugfs_add_file_to_list(encoder);
+}
+EXPORT_SYMBOL(drm_debugfs_encoder_add_file);
+
 static int connector_show(struct seq_file *m, void *data)
 {
 	struct drm_connector *connector = m->private;
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 1143bc7f3252..d6de6237cb4f 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -30,6 +30,7 @@
 #include <drm/drm_print.h>
 
 #include "drm_crtc_internal.h"
+#include "drm_internal.h"
 
 /**
  * DOC: overview
@@ -78,6 +79,7 @@ int drm_encoder_register_all(struct drm_device *dev)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)
 			return ret;
+		drm_debugfs_encoder_init(encoder);
 	}
 
 	return 0;
@@ -125,9 +127,12 @@ static int __drm_encoder_init(struct drm_device *dev,
 	}
 
 	INIT_LIST_HEAD(&encoder->bridge_chain);
+	INIT_LIST_HEAD(&encoder->debugfs_list);
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	encoder->index = dev->mode_config.num_encoder++;
 
+	mutex_init(&encoder->debugfs_mutex);
+
 out_put:
 	if (ret)
 		drm_mode_object_unregister(dev, &encoder->base);
@@ -197,6 +202,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);
 	list_del(&encoder->head);
+	mutex_destroy(&encoder->debugfs_mutex);
 	dev->mode_config.num_encoder--;
 
 	memset(encoder, 0, sizeof(*encoder));
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index dd9d7b8b45bd..363936ee8628 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -186,6 +186,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		     struct dentry *root);
 void drm_debugfs_connector_init(struct drm_connector *connector);
+void drm_debugfs_encoder_init(struct drm_encoder *encoder);
 void drm_debugfs_cleanup(struct drm_minor *minor);
 void drm_debugfs_late_register(struct drm_device *dev);
 void drm_debugfs_connector_add(struct drm_connector *connector);
@@ -204,6 +205,10 @@ static inline void drm_debugfs_connector_init(struct drm_connector *connector)
 {
 }
 
+static inline void drm_debugfs_encoder_init(struct drm_encoder *encoder)
+{
+}
+
 static inline void drm_debugfs_cleanup(struct drm_minor *minor)
 {
 }
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index c09c82274622..677ed3fee5e1 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -139,6 +139,23 @@ struct drm_debugfs_connector_entry {
 	struct list_head list;
 };
 
+/**
+ * struct drm_debugfs_encoder_entry - Per-encoder debugfs node structure
+ *
+ * This structure represents a debugfs file, as an instantiation of a &struct
+ * drm_debugfs_info on a &struct drm_encoder.
+ */
+struct drm_debugfs_encoder_entry {
+	/** @encoder: &struct drm_encoder for this node. */
+	struct drm_encoder *encoder;
+
+	/** @file: Template for this node. */
+	struct drm_debugfs_info file;
+
+	/** @list: Linked list of all encoder nodes. */
+	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,
@@ -154,6 +171,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
 
 void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
 				    int (*show)(struct seq_file*, void*), void *data);
+
+void drm_debugfs_encoder_add_file(struct drm_encoder *encoder, const char *name,
+				  int (*show)(struct seq_file*, void*), void *data);
 #else
 static inline void drm_debugfs_create_files(const struct drm_info_list *files,
 					    int count, struct dentry *root,
@@ -181,6 +201,12 @@ static inline void drm_debugfs_connector_add_file(struct drm_connector *connecto
 						  int (*show)(struct seq_file*, void*),
 						  void *data)
 {}
+
+static inline void drm_debugfs_encoder_add_file(struct drm_encoder *encoder,
+						const char *name,
+						int (*show)(struct seq_file*, void*),
+						void *data)
+{}
 #endif
 
 #endif /* _DRM_DEBUGFS_H_ */
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 3a09682af685..38b73f2a4e38 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -182,6 +182,21 @@ struct drm_encoder {
 	 */
 	struct list_head bridge_chain;
 
+	/**
+	 * @debugfs_mutex:
+	 *
+	 * Protects &debugfs_list access.
+	 */
+	struct mutex debugfs_mutex;
+
+	/**
+	 * @debugfs_list:
+	 *
+	 * List of debugfs files to be created by the DRM encoder. The files
+	 * must be added during drm_encoder_register_all().
+	 */
+	struct list_head debugfs_list;
+
 	const struct drm_encoder_funcs *funcs;
 	const struct drm_encoder_helper_funcs *helper_private;
 };
-- 
2.39.0

Re: [PATCH 04/13] drm/debugfs: Create a debugfs infrastructure for encoders

[Jani Nikula]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Introduce the ability to add DRM debugfs files to a list managed by the
> encoder and, during drm_encoder_register_all(), all added files will be
> created at once.
>
> Moreover, introduce some typesafety as struct drm_debugfs_encoder_entry
> holds a drm_encoder instead of a drm_device. So, the drivers can get
> a encoder object directly from the struct drm_debugfs_encoder_entry
> in the show() callback.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c  | 36 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_encoder.c  |  6 ++++++
>  drivers/gpu/drm/drm_internal.h |  5 +++++
>  include/drm/drm_debugfs.h      | 26 ++++++++++++++++++++++++
>  include/drm/drm_encoder.h      | 15 ++++++++++++++
>  5 files changed, 88 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index d9ec1ed5a7ec..6a763fe1b031 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -36,6 +36,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_managed.h>
> @@ -271,6 +272,17 @@ void drm_debugfs_connector_init(struct drm_connector *connector)
>  	drm_create_file_from_list(connector);
>  }
>  
> +void drm_debugfs_encoder_init(struct drm_encoder *encoder)
> +{
> +	struct drm_minor *minor = encoder->dev->primary;
> +	struct drm_debugfs_encoder_entry *entry, *tmp;
> +
> +	if (!minor)
> +		return;
> +
> +	drm_create_file_from_list(encoder);
> +}

Because of the macro, this just looks like entry and tmp are unused
local variables.

> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 3a09682af685..38b73f2a4e38 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -182,6 +182,21 @@ struct drm_encoder {
>  	 */
>  	struct list_head bridge_chain;
>  
> +	/**
> +	 * @debugfs_mutex:
> +	 *
> +	 * Protects &debugfs_list access.
> +	 */
> +	struct mutex debugfs_mutex;
> +
> +	/**
> +	 * @debugfs_list:
> +	 *
> +	 * List of debugfs files to be created by the DRM encoder. The files
> +	 * must be added during drm_encoder_register_all().
> +	 */
> +	struct list_head debugfs_list;
> +

If you added an additional struct wrapper for the above debugfs stuff
(and actually defined it in a drm debugfs header where it belongs), and
added that to encoder, connector, etc., you could pass a pointer to
*that* to the drm_debugfs_add_file_to_list() and
drm_create_file_from_list() proper functions.

Less boilerplate, nicer functions, debugfs stuff grouped together and
defined in the .[ch] they're used in.

I think that would be much nicer.


BR,
Jani.



>  	const struct drm_encoder_funcs *funcs;
>  	const struct drm_encoder_helper_funcs *helper_private;
>  };

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH 05/13] drm/debugfs: Create a debugfs infrastructure for CRTC

[Maíra Canal]

Introduce the ability to add DRM debugfs files to a list managed by the
crtc and, during drm_crtc_register_all(), all added files will be
created at once.

Moreover, introduce some typesafety as struct drm_debugfs_crtc_entry
holds a drm_crtc instead of a drm_device. So, the drivers can get
a crtc object directly from the struct drm_debugfs_crtc_entry
in the show() callback.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_crtc.c     |  5 +++++
 drivers/gpu/drm/drm_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_internal.h |  5 +++++
 include/drm/drm_crtc.h         | 15 +++++++++++++++
 include/drm/drm_debugfs.h      | 25 +++++++++++++++++++++++++
 5 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index df9bf3c9206e..2953eef3e88e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -131,6 +131,8 @@ int drm_crtc_register_all(struct drm_device *dev)
 			ret = crtc->funcs->late_register(crtc);
 		if (ret)
 			return ret;
+
+		drm_debugfs_crtc_init(crtc);
 	}
 
 	return 0;
@@ -268,7 +270,9 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *
 	crtc->funcs = funcs;
 
 	INIT_LIST_HEAD(&crtc->commit_list);
+	INIT_LIST_HEAD(&crtc->debugfs_list);
 	spin_lock_init(&crtc->commit_lock);
+	mutex_init(&crtc->debugfs_mutex);
 
 	drm_modeset_lock_init(&crtc->mutex);
 	ret = drm_mode_object_add(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
@@ -508,6 +512,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 	crtc->gamma_store = NULL;
 
 	drm_modeset_lock_fini(&crtc->mutex);
+	mutex_destroy(&crtc->debugfs_mutex);
 
 	drm_mode_object_unregister(dev, &crtc->base);
 	list_del(&crtc->head);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6a763fe1b031..e1f71a03a581 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -261,6 +261,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	return 0;
 }
 
+void drm_debugfs_crtc_init(struct drm_crtc *crtc)
+{
+	struct drm_minor *minor = crtc->dev->primary;
+	struct drm_debugfs_crtc_entry *entry, *tmp;
+
+	if (!minor)
+		return;
+
+	drm_create_file_from_list(crtc);
+}
+
 void drm_debugfs_connector_init(struct drm_connector *connector)
 {
 	struct drm_minor *minor = connector->dev->primary;
@@ -392,6 +403,28 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
 }
 EXPORT_SYMBOL(drm_debugfs_add_files);
 
+/**
+ * drm_debugfs_crtc_add_file - Add a given file to the DRM crtc debugfs file list
+ * @crtc: DRM crtc object
+ * @name: debugfs file name
+ * @show: show callback
+ * @data: driver-private data, should not be device-specific
+ * Add a given file entry to the DRM crtc debugfs file list to be created on
+ * drm_debugfs_crtc_init().
+ */
+void drm_debugfs_crtc_add_file(struct drm_crtc *crtc, const char *name,
+			       int (*show)(struct seq_file*, void*), void *data)
+{
+	struct drm_debugfs_crtc_entry *entry = drmm_kzalloc(crtc->dev, sizeof(*entry),
+							    GFP_KERNEL);
+
+	if (!entry)
+		return;
+
+	drm_debugfs_add_file_to_list(crtc);
+}
+EXPORT_SYMBOL(drm_debugfs_crtc_add_file);
+
 /**
  * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
  * @connector: DRM connector object
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 363936ee8628..9be697f7f8f9 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
 #if defined(CONFIG_DEBUG_FS)
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		     struct dentry *root);
+void drm_debugfs_crtc_init(struct drm_crtc *crtc);
 void drm_debugfs_connector_init(struct drm_connector *connector);
 void drm_debugfs_encoder_init(struct drm_encoder *encoder);
 void drm_debugfs_cleanup(struct drm_minor *minor);
@@ -201,6 +202,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	return 0;
 }
 
+static inline void drm_debugfs_crtc_init(struct drm_crtc *crtc)
+{
+}
+
 static inline void drm_debugfs_connector_init(struct drm_connector *connector)
 {
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8e1cbc75143e..612928929646 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1139,6 +1139,21 @@ struct drm_crtc {
 	 */
 	struct dentry *debugfs_entry;
 
+	/**
+	 * @debugfs_mutex:
+	 *
+	 * Protects &debugfs_list access.
+	 */
+	struct mutex debugfs_mutex;
+
+	/**
+	 * @debugfs_list:
+	 *
+	 * List of debugfs files to be created by the DRM crtc. The files
+	 * must be added during drm_crtc_register_all().
+	 */
+	struct list_head debugfs_list;
+
 	/**
 	 * @crc:
 	 *
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index 677ed3fee5e1..47f23615139f 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -122,6 +122,23 @@ struct drm_debugfs_entry {
 	struct list_head list;
 };
 
+/**
+ * struct drm_debugfs_crtc_entry - Per-crtc debugfs node structure
+ *
+ * This structure represents a debugfs file, as an instantiation of a &struct
+ * drm_debugfs_info on a &struct drm_crtc.
+ */
+struct drm_debugfs_crtc_entry {
+	/** @crtc: &struct drm_crtc for this node. */
+	struct drm_crtc *crtc;
+
+	/** @file: Template for this node. */
+	struct drm_debugfs_info file;
+
+	/** @list: Linked list of all crtc nodes. */
+	struct list_head list;
+};
+
 /**
  * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
  *
@@ -169,6 +186,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
 void drm_debugfs_add_files(struct drm_device *dev,
 			   const struct drm_debugfs_info *files, int count);
 
+void drm_debugfs_crtc_add_file(struct drm_crtc *crtc, const char *name,
+			       int (*show)(struct seq_file*, void*), void *data);
+
 void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
 				    int (*show)(struct seq_file*, void*), void *data);
 
@@ -196,6 +216,11 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
 					 int count)
 {}
 
+static inline void drm_debugfs_crtc_add_file(struct drm_crtc *crtc, const char *name,
+					     int (*show)(struct seq_file*, void*),
+					     void *data)
+{}
+
 static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
 						  const char *name,
 						  int (*show)(struct seq_file*, void*),
-- 
2.39.0

[PATCH 06/13] drm/vc4: Split variable instantiation of vc4_debugfs_regset32()

[Maíra Canal]

Considering that there are multiple ways to instantiate a
drm_debugfs_entry, as kms object's debugfs entries were introduced,
split vc4_debugfs_regset32() into two functions: one to deal with the
drm_printer and the other to instantiate the proper variables.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_debugfs.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index fac624a663ea..80afc69200f0 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -32,27 +32,34 @@ vc4_debugfs_init(struct drm_minor *minor)
 	}
 }
 
-static int vc4_debugfs_regset32(struct seq_file *m, void *unused)
+static int vc4_debugfs_regset32(struct drm_device *drm, struct debugfs_regset32 *regset,
+				struct drm_printer *p)
 {
-	struct drm_debugfs_entry *entry = m->private;
-	struct drm_device *drm = entry->dev;
-	struct debugfs_regset32 *regset = entry->file.data;
-	struct drm_printer p = drm_seq_file_printer(m);
 	int idx;
 
 	if (!drm_dev_enter(drm, &idx))
 		return -ENODEV;
 
-	drm_print_regset32(&p, regset);
+	drm_print_regset32(p, regset);
 
 	drm_dev_exit(idx);
 
 	return 0;
 }
 
+static int vc4_debugfs_dev_regset32(struct seq_file *m, void *unused)
+{
+	struct drm_debugfs_entry *entry = m->private;
+	struct drm_device *drm = entry->dev;
+	struct debugfs_regset32 *regset = entry->file.data;
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	return vc4_debugfs_regset32(drm, regset, &p);
+}
+
 void vc4_debugfs_add_regset32(struct drm_device *drm,
 			      const char *name,
 			      struct debugfs_regset32 *regset)
 {
-	drm_debugfs_add_file(drm, name, vc4_debugfs_regset32, regset);
+	drm_debugfs_add_file(drm, name, vc4_debugfs_dev_regset32, regset);
 }
-- 
2.39.0

[PATCH 07/13] drm/vc4: Use the encoders' debugfs infrastructure

[Maíra Canal]

Replace the use of drm_debugfs_add_files() with the new
drm_debugfs_encoder_add_files() function, which centers the debugfs files
management on the drm_encoder instead of drm_device. Using this function
on late register callbacks is more adequate as the callback passes a
drm_encoder as parameter.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_debugfs.c | 17 +++++++++++++++++
 drivers/gpu/drm/vc4/vc4_dpi.c     |  3 +--
 drivers/gpu/drm/vc4/vc4_drv.h     |  8 ++++++++
 drivers/gpu/drm/vc4/vc4_dsi.c     |  3 +--
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  5 ++---
 drivers/gpu/drm/vc4/vc4_vec.c     |  3 +--
 6 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index 80afc69200f0..c71e4d6ec4c4 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -57,9 +57,26 @@ static int vc4_debugfs_dev_regset32(struct seq_file *m, void *unused)
 	return vc4_debugfs_regset32(drm, regset, &p);
 }
 
+static int vc4_debugfs_encoder_regset32(struct seq_file *m, void *unused)
+{
+	struct drm_debugfs_encoder_entry *entry = m->private;
+	struct drm_device *drm = entry->encoder->dev;
+	struct debugfs_regset32 *regset = entry->file.data;
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	return vc4_debugfs_regset32(drm, regset, &p);
+}
+
 void vc4_debugfs_add_regset32(struct drm_device *drm,
 			      const char *name,
 			      struct debugfs_regset32 *regset)
 {
 	drm_debugfs_add_file(drm, name, vc4_debugfs_dev_regset32, regset);
 }
+
+void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
+				      const char *name,
+				      struct debugfs_regset32 *regset)
+{
+	drm_debugfs_encoder_add_file(encoder, name, vc4_debugfs_encoder_regset32, regset);
+}
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index f518d6e59ed6..084f7825dfa4 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -265,10 +265,9 @@ static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = {
 
 static int vc4_dpi_late_register(struct drm_encoder *encoder)
 {
-	struct drm_device *drm = encoder->dev;
 	struct vc4_dpi *dpi = to_vc4_dpi(encoder);
 
-	vc4_debugfs_add_regset32(drm, "dpi_regs", &dpi->regset);
+	vc4_debugfs_encoder_add_regset32(encoder, "dpi_regs", &dpi->regset);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 95069bb16821..8aaa8d00bc45 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -969,12 +969,20 @@ void vc4_debugfs_init(struct drm_minor *minor);
 void vc4_debugfs_add_regset32(struct drm_device *drm,
 			      const char *filename,
 			      struct debugfs_regset32 *regset);
+void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
+				      const char *name,
+				      struct debugfs_regset32 *regset);
 #else
 
 static inline void vc4_debugfs_add_regset32(struct drm_device *drm,
 					    const char *filename,
 					    struct debugfs_regset32 *regset)
 {}
+
+static inline void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
+						    const char *name,
+						    struct debugfs_regset32 *regset)
+{}
 #endif
 
 /* vc4_drv.c */
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 2a71321b9222..00751b76bfe0 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1424,10 +1424,9 @@ static const struct drm_bridge_funcs vc4_dsi_bridge_funcs = {
 
 static int vc4_dsi_late_register(struct drm_encoder *encoder)
 {
-	struct drm_device *drm = encoder->dev;
 	struct vc4_dsi *dsi = to_vc4_dsi(encoder);
 
-	vc4_debugfs_add_regset32(drm, dsi->variant->debugfs_name, &dsi->regset);
+	vc4_debugfs_encoder_add_regset32(encoder, dsi->variant->debugfs_name, &dsi->regset);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 14628864487a..221cef11d4dd 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2002,12 +2002,11 @@ static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
 
 static int vc4_hdmi_late_register(struct drm_encoder *encoder)
 {
-	struct drm_device *drm = encoder->dev;
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	const struct vc4_hdmi_variant *variant = vc4_hdmi->variant;
 
-	drm_debugfs_add_file(drm, variant->debugfs_name,
-			     vc4_hdmi_debugfs_regs, vc4_hdmi);
+	drm_debugfs_encoder_add_file(encoder, variant->debugfs_name,
+				     vc4_hdmi_debugfs_regs, vc4_hdmi);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index a3782d05cd66..4c5bd733d524 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -716,10 +716,9 @@ static const struct drm_encoder_helper_funcs vc4_vec_encoder_helper_funcs = {
 
 static int vc4_vec_late_register(struct drm_encoder *encoder)
 {
-	struct drm_device *drm = encoder->dev;
 	struct vc4_vec *vec = encoder_to_vc4_vec(encoder);
 
-	vc4_debugfs_add_regset32(drm, "vec_regs", &vec->regset);
+	vc4_debugfs_encoder_add_regset32(encoder, "vec_regs", &vec->regset);
 
 	return 0;
 }
-- 
2.39.0

Re: [PATCH 07/13] drm/vc4: Use the encoders' debugfs infrastructure

[Jani Nikula]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Replace the use of drm_debugfs_add_files() with the new
> drm_debugfs_encoder_add_files() function, which centers the debugfs files
> management on the drm_encoder instead of drm_device. Using this function
> on late register callbacks is more adequate as the callback passes a
> drm_encoder as parameter.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/vc4/vc4_debugfs.c | 17 +++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_dpi.c     |  3 +--
>  drivers/gpu/drm/vc4/vc4_drv.h     |  8 ++++++++
>  drivers/gpu/drm/vc4/vc4_dsi.c     |  3 +--
>  drivers/gpu/drm/vc4/vc4_hdmi.c    |  5 ++---
>  drivers/gpu/drm/vc4/vc4_vec.c     |  3 +--
>  6 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> index 80afc69200f0..c71e4d6ec4c4 100644
> --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> @@ -57,9 +57,26 @@ static int vc4_debugfs_dev_regset32(struct seq_file *m, void *unused)
>  	return vc4_debugfs_regset32(drm, regset, &p);
>  }
>  
> +static int vc4_debugfs_encoder_regset32(struct seq_file *m, void *unused)
> +{
> +	struct drm_debugfs_encoder_entry *entry = m->private;
> +	struct drm_device *drm = entry->encoder->dev;

Feels like struct drm_debugfs_encoder_entry should be an implementation
detail. Maybe add helpers to get the encoder/connector/etc pointer, and
keep struct drm_debugfs_encoder_entry internal to the debugfs
implementation?

	struct drm_device *drm = drm_debugfs_something_encoder(m->private)->dev;

However, even cooler would be if we could have the debugfs code wrap the
calls, and you could have:

	static int vc4_debugfs_encoder_regset32(struct drm_encoder *encoder);

struct drm_debugfs_encoder_entry could have a function pointer for the
above, and there'd be a simple wrapper in debugfs code:

static int encoder_debugfs_show(struct seq_file *m, void *unused)
{
	struct drm_debugfs_encoder_entry *entry = m->private;
	struct drm_encoder *encoder = entry->encoder;

	return entry->show(encoder);
}

drm_debugfs_encoder_add_file() would fill in entry->show, and add that
as the show function for core debugfs code.

Ditto for connector/crtc/etc.

This would make all of the drm debugfs functions so much nicer.

BR,
Jani.


> +	struct debugfs_regset32 *regset = entry->file.data;
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	return vc4_debugfs_regset32(drm, regset, &p);
> +}
> +
>  void vc4_debugfs_add_regset32(struct drm_device *drm,
>  			      const char *name,
>  			      struct debugfs_regset32 *regset)
>  {
>  	drm_debugfs_add_file(drm, name, vc4_debugfs_dev_regset32, regset);
>  }
> +
> +void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> +				      const char *name,
> +				      struct debugfs_regset32 *regset)
> +{
> +	drm_debugfs_encoder_add_file(encoder, name, vc4_debugfs_encoder_regset32, regset);
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index f518d6e59ed6..084f7825dfa4 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -265,10 +265,9 @@ static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = {
>  
>  static int vc4_dpi_late_register(struct drm_encoder *encoder)
>  {
> -	struct drm_device *drm = encoder->dev;
>  	struct vc4_dpi *dpi = to_vc4_dpi(encoder);
>  
> -	vc4_debugfs_add_regset32(drm, "dpi_regs", &dpi->regset);
> +	vc4_debugfs_encoder_add_regset32(encoder, "dpi_regs", &dpi->regset);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 95069bb16821..8aaa8d00bc45 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -969,12 +969,20 @@ void vc4_debugfs_init(struct drm_minor *minor);
>  void vc4_debugfs_add_regset32(struct drm_device *drm,
>  			      const char *filename,
>  			      struct debugfs_regset32 *regset);
> +void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> +				      const char *name,
> +				      struct debugfs_regset32 *regset);
>  #else
>  
>  static inline void vc4_debugfs_add_regset32(struct drm_device *drm,
>  					    const char *filename,
>  					    struct debugfs_regset32 *regset)
>  {}
> +
> +static inline void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> +						    const char *name,
> +						    struct debugfs_regset32 *regset)
> +{}
>  #endif
>  
>  /* vc4_drv.c */
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 2a71321b9222..00751b76bfe0 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1424,10 +1424,9 @@ static const struct drm_bridge_funcs vc4_dsi_bridge_funcs = {
>  
>  static int vc4_dsi_late_register(struct drm_encoder *encoder)
>  {
> -	struct drm_device *drm = encoder->dev;
>  	struct vc4_dsi *dsi = to_vc4_dsi(encoder);
>  
> -	vc4_debugfs_add_regset32(drm, dsi->variant->debugfs_name, &dsi->regset);
> +	vc4_debugfs_encoder_add_regset32(encoder, dsi->variant->debugfs_name, &dsi->regset);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 14628864487a..221cef11d4dd 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2002,12 +2002,11 @@ static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
>  
>  static int vc4_hdmi_late_register(struct drm_encoder *encoder)
>  {
> -	struct drm_device *drm = encoder->dev;
>  	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
>  	const struct vc4_hdmi_variant *variant = vc4_hdmi->variant;
>  
> -	drm_debugfs_add_file(drm, variant->debugfs_name,
> -			     vc4_hdmi_debugfs_regs, vc4_hdmi);
> +	drm_debugfs_encoder_add_file(encoder, variant->debugfs_name,
> +				     vc4_hdmi_debugfs_regs, vc4_hdmi);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index a3782d05cd66..4c5bd733d524 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -716,10 +716,9 @@ static const struct drm_encoder_helper_funcs vc4_vec_encoder_helper_funcs = {
>  
>  static int vc4_vec_late_register(struct drm_encoder *encoder)
>  {
> -	struct drm_device *drm = encoder->dev;
>  	struct vc4_vec *vec = encoder_to_vc4_vec(encoder);
>  
> -	vc4_debugfs_add_regset32(drm, "vec_regs", &vec->regset);
> +	vc4_debugfs_encoder_add_regset32(encoder, "vec_regs", &vec->regset);
>  
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 07/13] drm/vc4: Use the encoders' debugfs infrastructure

[Daniel Vetter]

On Thu, Jan 12, 2023 at 11:19:37AM +0200, Jani Nikula wrote:
> On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> > Replace the use of drm_debugfs_add_files() with the new
> > drm_debugfs_encoder_add_files() function, which centers the debugfs files
> > management on the drm_encoder instead of drm_device. Using this function
> > on late register callbacks is more adequate as the callback passes a
> > drm_encoder as parameter.
> >
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_debugfs.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/vc4/vc4_dpi.c     |  3 +--
> >  drivers/gpu/drm/vc4/vc4_drv.h     |  8 ++++++++
> >  drivers/gpu/drm/vc4/vc4_dsi.c     |  3 +--
> >  drivers/gpu/drm/vc4/vc4_hdmi.c    |  5 ++---
> >  drivers/gpu/drm/vc4/vc4_vec.c     |  3 +--
> >  6 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> > index 80afc69200f0..c71e4d6ec4c4 100644
> > --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> > +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> > @@ -57,9 +57,26 @@ static int vc4_debugfs_dev_regset32(struct seq_file *m, void *unused)
> >  	return vc4_debugfs_regset32(drm, regset, &p);
> >  }
> >  
> > +static int vc4_debugfs_encoder_regset32(struct seq_file *m, void *unused)
> > +{
> > +	struct drm_debugfs_encoder_entry *entry = m->private;
> > +	struct drm_device *drm = entry->encoder->dev;
> 
> Feels like struct drm_debugfs_encoder_entry should be an implementation
> detail. Maybe add helpers to get the encoder/connector/etc pointer, and
> keep struct drm_debugfs_encoder_entry internal to the debugfs
> implementation?
> 
> 	struct drm_device *drm = drm_debugfs_something_encoder(m->private)->dev;
> 
> However, even cooler would be if we could have the debugfs code wrap the
> calls, and you could have:
> 
> 	static int vc4_debugfs_encoder_regset32(struct drm_encoder *encoder);
> 
> struct drm_debugfs_encoder_entry could have a function pointer for the
> above, and there'd be a simple wrapper in debugfs code:
> 
> static int encoder_debugfs_show(struct seq_file *m, void *unused)
> {
> 	struct drm_debugfs_encoder_entry *entry = m->private;
> 	struct drm_encoder *encoder = entry->encoder;
> 
> 	return entry->show(encoder);
> }
> 
> drm_debugfs_encoder_add_file() would fill in entry->show, and add that
> as the show function for core debugfs code.
> 
> Ditto for connector/crtc/etc.
> 
> This would make all of the drm debugfs functions so much nicer.

Yeah this is what I mean with "we should pass the right struct pointer
explicitly". I think at this point the drm debugfs wrappers actually start
to really pay for themselves, because you can make nice clean debugfs
show/write functions for various little things.
-Daniel

> 
> BR,
> Jani.
> 
> 
> > +	struct debugfs_regset32 *regset = entry->file.data;
> > +	struct drm_printer p = drm_seq_file_printer(m);
> > +
> > +	return vc4_debugfs_regset32(drm, regset, &p);
> > +}
> > +
> >  void vc4_debugfs_add_regset32(struct drm_device *drm,
> >  			      const char *name,
> >  			      struct debugfs_regset32 *regset)
> >  {
> >  	drm_debugfs_add_file(drm, name, vc4_debugfs_dev_regset32, regset);
> >  }
> > +
> > +void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> > +				      const char *name,
> > +				      struct debugfs_regset32 *regset)
> > +{
> > +	drm_debugfs_encoder_add_file(encoder, name, vc4_debugfs_encoder_regset32, regset);
> > +}
> > diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> > index f518d6e59ed6..084f7825dfa4 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> > @@ -265,10 +265,9 @@ static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = {
> >  
> >  static int vc4_dpi_late_register(struct drm_encoder *encoder)
> >  {
> > -	struct drm_device *drm = encoder->dev;
> >  	struct vc4_dpi *dpi = to_vc4_dpi(encoder);
> >  
> > -	vc4_debugfs_add_regset32(drm, "dpi_regs", &dpi->regset);
> > +	vc4_debugfs_encoder_add_regset32(encoder, "dpi_regs", &dpi->regset);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index 95069bb16821..8aaa8d00bc45 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -969,12 +969,20 @@ void vc4_debugfs_init(struct drm_minor *minor);
> >  void vc4_debugfs_add_regset32(struct drm_device *drm,
> >  			      const char *filename,
> >  			      struct debugfs_regset32 *regset);
> > +void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> > +				      const char *name,
> > +				      struct debugfs_regset32 *regset);
> >  #else
> >  
> >  static inline void vc4_debugfs_add_regset32(struct drm_device *drm,
> >  					    const char *filename,
> >  					    struct debugfs_regset32 *regset)
> >  {}
> > +
> > +static inline void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
> > +						    const char *name,
> > +						    struct debugfs_regset32 *regset)
> > +{}
> >  #endif
> >  
> >  /* vc4_drv.c */
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index 2a71321b9222..00751b76bfe0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1424,10 +1424,9 @@ static const struct drm_bridge_funcs vc4_dsi_bridge_funcs = {
> >  
> >  static int vc4_dsi_late_register(struct drm_encoder *encoder)
> >  {
> > -	struct drm_device *drm = encoder->dev;
> >  	struct vc4_dsi *dsi = to_vc4_dsi(encoder);
> >  
> > -	vc4_debugfs_add_regset32(drm, dsi->variant->debugfs_name, &dsi->regset);
> > +	vc4_debugfs_encoder_add_regset32(encoder, dsi->variant->debugfs_name, &dsi->regset);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 14628864487a..221cef11d4dd 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -2002,12 +2002,11 @@ static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
> >  
> >  static int vc4_hdmi_late_register(struct drm_encoder *encoder)
> >  {
> > -	struct drm_device *drm = encoder->dev;
> >  	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
> >  	const struct vc4_hdmi_variant *variant = vc4_hdmi->variant;
> >  
> > -	drm_debugfs_add_file(drm, variant->debugfs_name,
> > -			     vc4_hdmi_debugfs_regs, vc4_hdmi);
> > +	drm_debugfs_encoder_add_file(encoder, variant->debugfs_name,
> > +				     vc4_hdmi_debugfs_regs, vc4_hdmi);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> > index a3782d05cd66..4c5bd733d524 100644
> > --- a/drivers/gpu/drm/vc4/vc4_vec.c
> > +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> > @@ -716,10 +716,9 @@ static const struct drm_encoder_helper_funcs vc4_vec_encoder_helper_funcs = {
> >  
> >  static int vc4_vec_late_register(struct drm_encoder *encoder)
> >  {
> > -	struct drm_device *drm = encoder->dev;
> >  	struct vc4_vec *vec = encoder_to_vc4_vec(encoder);
> >  
> > -	vc4_debugfs_add_regset32(drm, "vec_regs", &vec->regset);
> > +	vc4_debugfs_encoder_add_regset32(encoder, "vec_regs", &vec->regset);
> >  
> >  	return 0;
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 08/13] drm/vc4: Use the crtc's debugfs infrastructure

[Maíra Canal]

Replace the use of drm_debugfs_add_files() with the new
drm_debugfs_crtc_add_files() function, which centers the debugfs files
management on the drm_crtc instead of drm_device. Using this function
on late register callbacks is more adequate as the callback passes a
drm_crtc as parameter.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c    |  5 ++---
 drivers/gpu/drm/vc4/vc4_debugfs.c | 17 +++++++++++++++++
 drivers/gpu/drm/vc4/vc4_drv.h     |  8 ++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index cdc0559221f0..3e5c71c0bed3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1104,12 +1104,11 @@ void vc4_crtc_reset(struct drm_crtc *crtc)
 
 int vc4_crtc_late_register(struct drm_crtc *crtc)
 {
-	struct drm_device *drm = crtc->dev;
 	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 	const struct vc4_crtc_data *crtc_data = vc4_crtc_to_vc4_crtc_data(vc4_crtc);
 
-	vc4_debugfs_add_regset32(drm, crtc_data->debugfs_name,
-				 &vc4_crtc->regset);
+	vc4_debugfs_crtc_add_regset32(crtc, crtc_data->debugfs_name,
+				      &vc4_crtc->regset);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index c71e4d6ec4c4..33023ae32ddf 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -57,6 +57,16 @@ static int vc4_debugfs_dev_regset32(struct seq_file *m, void *unused)
 	return vc4_debugfs_regset32(drm, regset, &p);
 }
 
+static int vc4_debugfs_crtc_regset32(struct seq_file *m, void *unused)
+{
+	struct drm_debugfs_crtc_entry *entry = m->private;
+	struct drm_device *drm = entry->crtc->dev;
+	struct debugfs_regset32 *regset = entry->file.data;
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	return vc4_debugfs_regset32(drm, regset, &p);
+}
+
 static int vc4_debugfs_encoder_regset32(struct seq_file *m, void *unused)
 {
 	struct drm_debugfs_encoder_entry *entry = m->private;
@@ -74,6 +84,13 @@ void vc4_debugfs_add_regset32(struct drm_device *drm,
 	drm_debugfs_add_file(drm, name, vc4_debugfs_dev_regset32, regset);
 }
 
+void vc4_debugfs_crtc_add_regset32(struct drm_crtc *crtc,
+				   const char *name,
+				   struct debugfs_regset32 *regset)
+{
+	drm_debugfs_crtc_add_file(crtc, name, vc4_debugfs_crtc_regset32, regset);
+}
+
 void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
 				      const char *name,
 				      struct debugfs_regset32 *regset)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 8aaa8d00bc45..9a1b777afee2 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -969,6 +969,9 @@ void vc4_debugfs_init(struct drm_minor *minor);
 void vc4_debugfs_add_regset32(struct drm_device *drm,
 			      const char *filename,
 			      struct debugfs_regset32 *regset);
+void vc4_debugfs_crtc_add_regset32(struct drm_crtc *crtc,
+				   const char *name,
+				   struct debugfs_regset32 *regset);
 void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
 				      const char *name,
 				      struct debugfs_regset32 *regset);
@@ -979,6 +982,11 @@ static inline void vc4_debugfs_add_regset32(struct drm_device *drm,
 					    struct debugfs_regset32 *regset)
 {}
 
+static inline void vc4_debugfs_crtc_add_regset32(struct drm_crtc *crtc,
+						 const char *name,
+						 struct debugfs_regset32 *regset)
+{}
+
 static inline void vc4_debugfs_encoder_add_regset32(struct drm_encoder *encoder,
 						    const char *name,
 						    struct debugfs_regset32 *regset)
-- 
2.39.0

[PATCH 09/13] drm/sti: Use the crtc's debugfs infrastructure

[Maíra Canal]

Replace the use of drm_debugfs_add_files() with the new
drm_debugfs_crtc_add_files() function, which centers the debugfs files
management on the drm_crtc instead of drm_device. Using this function
on late register callbacks is more adequate as the callback passes a
drm_crtc as parameter.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/sti/sti_compositor.c |  6 +++---
 drivers/gpu/drm/sti/sti_compositor.h |  2 +-
 drivers/gpu/drm/sti/sti_crtc.c       |  2 +-
 drivers/gpu/drm/sti/sti_mixer.c      | 31 +++++-----------------------
 drivers/gpu/drm/sti/sti_mixer.h      |  2 +-
 drivers/gpu/drm/sti/sti_vid.c        | 19 ++++-------------
 drivers/gpu/drm/sti/sti_vid.h        |  2 +-
 7 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_compositor.c b/drivers/gpu/drm/sti/sti_compositor.c
index 142a8e1b4436..822da9d2879c 100644
--- a/drivers/gpu/drm/sti/sti_compositor.c
+++ b/drivers/gpu/drm/sti/sti_compositor.c
@@ -44,17 +44,17 @@ static const struct sti_compositor_data stih407_compositor_data = {
 };
 
 void sti_compositor_debugfs_init(struct sti_compositor *compo,
-				 struct drm_minor *minor)
+				 struct drm_crtc *crtc)
 {
 	unsigned int i;
 
 	for (i = 0; i < STI_MAX_VID; i++)
 		if (compo->vid[i])
-			vid_debugfs_init(compo->vid[i], minor);
+			vid_debugfs_init(compo->vid[i], crtc);
 
 	for (i = 0; i < STI_MAX_MIXER; i++)
 		if (compo->mixer[i])
-			sti_mixer_debugfs_init(compo->mixer[i], minor);
+			sti_mixer_debugfs_init(compo->mixer[i], crtc);
 }
 
 static int sti_compositor_bind(struct device *dev,
diff --git a/drivers/gpu/drm/sti/sti_compositor.h b/drivers/gpu/drm/sti/sti_compositor.h
index 25bb01bdd013..f5e9a2e4b6b5 100644
--- a/drivers/gpu/drm/sti/sti_compositor.h
+++ b/drivers/gpu/drm/sti/sti_compositor.h
@@ -80,6 +80,6 @@ struct sti_compositor {
 };
 
 void sti_compositor_debugfs_init(struct sti_compositor *compo,
-				 struct drm_minor *minor);
+				 struct drm_crtc *crtc);
 
 #endif
diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c
index 3c7154f2d5f3..176c74db8bb7 100644
--- a/drivers/gpu/drm/sti/sti_crtc.c
+++ b/drivers/gpu/drm/sti/sti_crtc.c
@@ -318,7 +318,7 @@ static int sti_crtc_late_register(struct drm_crtc *crtc)
 	struct sti_compositor *compo = dev_get_drvdata(mixer->dev);
 
 	if (drm_crtc_index(crtc) == 0)
-		sti_compositor_debugfs_init(compo, crtc->dev->primary);
+		sti_compositor_debugfs_init(compo, crtc);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/sti/sti_mixer.c b/drivers/gpu/drm/sti/sti_mixer.c
index 7e5f14646625..ce775c64db2e 100644
--- a/drivers/gpu/drm/sti/sti_mixer.c
+++ b/drivers/gpu/drm/sti/sti_mixer.c
@@ -147,8 +147,8 @@ static void mixer_dbg_mxn(struct seq_file *s, void *addr)
 
 static int mixer_dbg_show(struct seq_file *s, void *arg)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_mixer *mixer = (struct sti_mixer *)node->info_ent->data;
+	struct drm_debugfs_crtc_entry *entry = s->private;
+	struct sti_mixer *mixer = (struct sti_mixer *)entry->file.data;
 
 	seq_printf(s, "%s: (vaddr = 0x%p)",
 		   sti_mixer_to_str(mixer), mixer->regs);
@@ -170,39 +170,18 @@ static int mixer_dbg_show(struct seq_file *s, void *arg)
 	return 0;
 }
 
-static struct drm_info_list mixer0_debugfs_files[] = {
-	{ "mixer_main", mixer_dbg_show, 0, NULL },
-};
-
-static struct drm_info_list mixer1_debugfs_files[] = {
-	{ "mixer_aux", mixer_dbg_show, 0, NULL },
-};
-
-void sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_minor *minor)
+void sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_crtc *crtc)
 {
-	unsigned int i;
-	struct drm_info_list *mixer_debugfs_files;
-	int nb_files;
-
 	switch (mixer->id) {
 	case STI_MIXER_MAIN:
-		mixer_debugfs_files = mixer0_debugfs_files;
-		nb_files = ARRAY_SIZE(mixer0_debugfs_files);
+		drm_debugfs_crtc_add_file(crtc, "mixer_main", mixer_dbg_show, mixer);
 		break;
 	case STI_MIXER_AUX:
-		mixer_debugfs_files = mixer1_debugfs_files;
-		nb_files = ARRAY_SIZE(mixer1_debugfs_files);
+		drm_debugfs_crtc_add_file(crtc, "mixer_aux", mixer_dbg_show, mixer);
 		break;
 	default:
 		return;
 	}
-
-	for (i = 0; i < nb_files; i++)
-		mixer_debugfs_files[i].data = mixer;
-
-	drm_debugfs_create_files(mixer_debugfs_files,
-				 nb_files,
-				 minor->debugfs_root, minor);
 }
 
 void sti_mixer_set_background_status(struct sti_mixer *mixer, bool enable)
diff --git a/drivers/gpu/drm/sti/sti_mixer.h b/drivers/gpu/drm/sti/sti_mixer.h
index ab06beb7b258..973f7058092b 100644
--- a/drivers/gpu/drm/sti/sti_mixer.h
+++ b/drivers/gpu/drm/sti/sti_mixer.h
@@ -58,7 +58,7 @@ int sti_mixer_active_video_area(struct sti_mixer *mixer,
 
 void sti_mixer_set_background_status(struct sti_mixer *mixer, bool enable);
 
-void sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_minor *minor);
+void sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_crtc *crtc);
 
 /* depth in Cross-bar control = z order */
 #define GAM_MIXER_NB_DEPTH_LEVEL 6
diff --git a/drivers/gpu/drm/sti/sti_vid.c b/drivers/gpu/drm/sti/sti_vid.c
index 2d818397918d..98f2f4c8c2db 100644
--- a/drivers/gpu/drm/sti/sti_vid.c
+++ b/drivers/gpu/drm/sti/sti_vid.c
@@ -92,8 +92,8 @@ static void vid_dbg_mst(struct seq_file *s, int val)
 
 static int vid_dbg_show(struct seq_file *s, void *arg)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_vid *vid = (struct sti_vid *)node->info_ent->data;
+	struct drm_debugfs_crtc_entry *entry = s->private;
+	struct sti_vid *vid = (struct sti_vid *)entry->file.data;
 
 	seq_printf(s, "VID: (vaddr= 0x%p)", vid->regs);
 
@@ -120,20 +120,9 @@ static int vid_dbg_show(struct seq_file *s, void *arg)
 	return 0;
 }
 
-static struct drm_info_list vid_debugfs_files[] = {
-	{ "vid", vid_dbg_show, 0, NULL },
-};
-
-void vid_debugfs_init(struct sti_vid *vid, struct drm_minor *minor)
+void vid_debugfs_init(struct sti_vid *vid, struct drm_crtc *crtc)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(vid_debugfs_files); i++)
-		vid_debugfs_files[i].data = vid;
-
-	drm_debugfs_create_files(vid_debugfs_files,
-				 ARRAY_SIZE(vid_debugfs_files),
-				 minor->debugfs_root, minor);
+	drm_debugfs_crtc_add_file(crtc, "vid", vid_dbg_show, vid);
 }
 
 void sti_vid_commit(struct sti_vid *vid,
diff --git a/drivers/gpu/drm/sti/sti_vid.h b/drivers/gpu/drm/sti/sti_vid.h
index 991849ba50b5..a14577a8df48 100644
--- a/drivers/gpu/drm/sti/sti_vid.h
+++ b/drivers/gpu/drm/sti/sti_vid.h
@@ -26,6 +26,6 @@ void sti_vid_disable(struct sti_vid *vid);
 struct sti_vid *sti_vid_create(struct device *dev, struct drm_device *drm_dev,
 			       int id, void __iomem *baseaddr);
 
-void vid_debugfs_init(struct sti_vid *vid, struct drm_minor *minor);
+void vid_debugfs_init(struct sti_vid *vid, struct drm_crtc *crtc);
 
 #endif
-- 
2.39.0

[PATCH 10/13] drm/sti: Use the connectors' debugfs infrastructure

[Maíra Canal]

Replace the use of drm_debugfs_add_files() with the new
drm_debugfs_connector_add_files() function, which centers the debugfs files
management on the drm_connector instead of drm_device. Using this function
on late register callbacks is more adequate as the callback passes a
drm_connector as parameter.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/sti/sti_dvo.c  | 21 +++++----------------
 drivers/gpu/drm/sti/sti_hda.c  | 21 +++++----------------
 drivers/gpu/drm/sti/sti_hdmi.c | 21 +++++----------------
 3 files changed, 15 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index f3a5616b7daf..b617c288b893 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -178,8 +178,8 @@ static void dvo_dbg_awg_microcode(struct seq_file *s, void __iomem *reg)
 
 static int dvo_dbg_show(struct seq_file *s, void *data)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_dvo *dvo = (struct sti_dvo *)node->info_ent->data;
+	struct drm_debugfs_connector_entry *entry = s->private;
+	struct sti_dvo *dvo = (struct sti_dvo *)entry->file.data;
 
 	seq_printf(s, "DVO: (vaddr = 0x%p)", dvo->regs);
 	DBGFS_DUMP(DVO_AWG_DIGSYNC_CTRL);
@@ -192,20 +192,9 @@ static int dvo_dbg_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-static struct drm_info_list dvo_debugfs_files[] = {
-	{ "dvo", dvo_dbg_show, 0, NULL },
-};
-
-static void dvo_debugfs_init(struct sti_dvo *dvo, struct drm_minor *minor)
+static void dvo_debugfs_init(struct sti_dvo *dvo, struct drm_connector *connector)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(dvo_debugfs_files); i++)
-		dvo_debugfs_files[i].data = dvo;
-
-	drm_debugfs_create_files(dvo_debugfs_files,
-				 ARRAY_SIZE(dvo_debugfs_files),
-				 minor->debugfs_root, minor);
+	drm_debugfs_connector_add_file(connector, "dvo", dvo_dbg_show, dvo);
 }
 
 static void sti_dvo_disable(struct drm_bridge *bridge)
@@ -403,7 +392,7 @@ static int sti_dvo_late_register(struct drm_connector *connector)
 		= to_sti_dvo_connector(connector);
 	struct sti_dvo *dvo = dvo_connector->dvo;
 
-	dvo_debugfs_init(dvo, dvo->drm_dev->primary);
+	dvo_debugfs_init(dvo, connector);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index ec6656b9ee7c..9560f29db6e1 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -345,8 +345,8 @@ static void hda_dbg_video_dacs_ctrl(struct seq_file *s, void __iomem *reg)
 
 static int hda_dbg_show(struct seq_file *s, void *data)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_hda *hda = (struct sti_hda *)node->info_ent->data;
+	struct drm_debugfs_connector_entry *entry = s->private;
+	struct sti_hda *hda = (struct sti_hda *)entry->file.data;
 
 	seq_printf(s, "HD Analog: (vaddr = 0x%p)", hda->regs);
 	DBGFS_DUMP(HDA_ANA_CFG);
@@ -364,20 +364,9 @@ static int hda_dbg_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-static struct drm_info_list hda_debugfs_files[] = {
-	{ "hda", hda_dbg_show, 0, NULL },
-};
-
-static void hda_debugfs_init(struct sti_hda *hda, struct drm_minor *minor)
+static void hda_debugfs_init(struct sti_hda *hda, struct drm_connector *connector)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(hda_debugfs_files); i++)
-		hda_debugfs_files[i].data = hda;
-
-	drm_debugfs_create_files(hda_debugfs_files,
-				 ARRAY_SIZE(hda_debugfs_files),
-				 minor->debugfs_root, minor);
+	drm_debugfs_connector_add_file(connector, "hda", hda_dbg_show, hda);
 }
 
 /**
@@ -643,7 +632,7 @@ static int sti_hda_late_register(struct drm_connector *connector)
 		= to_sti_hda_connector(connector);
 	struct sti_hda *hda = hda_connector->hda;
 
-	hda_debugfs_init(hda, hda->drm_dev->primary);
+	hda_debugfs_init(hda, connector);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index fcc2194869d6..9b82d9b7db6a 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -668,8 +668,8 @@ static void hdmi_dbg_sw_di_cfg(struct seq_file *s, int val)
 
 static int hdmi_dbg_show(struct seq_file *s, void *data)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_hdmi *hdmi = (struct sti_hdmi *)node->info_ent->data;
+	struct drm_debugfs_connector_entry *entry = s->private;
+	struct sti_hdmi *hdmi = (struct sti_hdmi *)entry->file.data;
 
 	seq_printf(s, "HDMI: (vaddr = 0x%p)", hdmi->regs);
 	DBGFS_DUMP("\n", HDMI_CFG);
@@ -730,20 +730,9 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-static struct drm_info_list hdmi_debugfs_files[] = {
-	{ "hdmi", hdmi_dbg_show, 0, NULL },
-};
-
-static void hdmi_debugfs_init(struct sti_hdmi *hdmi, struct drm_minor *minor)
+static void hdmi_debugfs_init(struct sti_hdmi *hdmi, struct drm_connector *connector)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(hdmi_debugfs_files); i++)
-		hdmi_debugfs_files[i].data = hdmi;
-
-	drm_debugfs_create_files(hdmi_debugfs_files,
-				 ARRAY_SIZE(hdmi_debugfs_files),
-				 minor->debugfs_root, minor);
+	drm_debugfs_connector_add_file(connector, "hdmi", hdmi_dbg_show, hdmi);
 }
 
 static void sti_hdmi_disable(struct drm_bridge *bridge)
@@ -1120,7 +1109,7 @@ static int sti_hdmi_late_register(struct drm_connector *connector)
 		= to_sti_hdmi_connector(connector);
 	struct sti_hdmi *hdmi = hdmi_connector->hdmi;
 
-	hdmi_debugfs_init(hdmi, hdmi->drm_dev->primary);
+	hdmi_debugfs_init(hdmi, connector);
 
 	return 0;
 }
-- 
2.39.0

[PATCH 11/13] drm/sti: Use the encoders' debugfs infrastructure

[Maíra Canal]

Replace the use of drm_debugfs_add_files() with the new
drm_debugfs_encoder_add_files() function, which centers the debugfs files
management on the drm_encoder instead of drm_device. Using this function
on late register callbacks is more adequate as the callback passes a
drm_encoder as parameter.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/sti/sti_tvout.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 2499715a69b7..5601888caec4 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -496,8 +496,8 @@ static void tvout_dbg_hd_dac_cfg(struct seq_file *s, int val)
 
 static int tvout_dbg_show(struct seq_file *s, void *data)
 {
-	struct drm_info_node *node = s->private;
-	struct sti_tvout *tvout = (struct sti_tvout *)node->info_ent->data;
+	struct drm_debugfs_encoder_entry *entry = s->private;
+	struct sti_tvout *tvout = (struct sti_tvout *)entry->file.data;
 	struct drm_crtc *crtc;
 
 	seq_printf(s, "TVOUT: (vaddr = 0x%p)", tvout->regs);
@@ -566,20 +566,9 @@ static int tvout_dbg_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-static struct drm_info_list tvout_debugfs_files[] = {
-	{ "tvout", tvout_dbg_show, 0, NULL },
-};
-
-static void tvout_debugfs_init(struct sti_tvout *tvout, struct drm_minor *minor)
+static void tvout_debugfs_init(struct sti_tvout *tvout, struct drm_encoder *encoder)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(tvout_debugfs_files); i++)
-		tvout_debugfs_files[i].data = tvout;
-
-	drm_debugfs_create_files(tvout_debugfs_files,
-				 ARRAY_SIZE(tvout_debugfs_files),
-				 minor->debugfs_root, minor);
+	drm_debugfs_encoder_add_file(encoder, "tvout", tvout_dbg_show, tvout);
 }
 
 static void sti_tvout_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -607,7 +596,7 @@ static int sti_tvout_late_register(struct drm_encoder *encoder)
 	if (tvout->debugfs_registered)
 		return 0;
 
-	tvout_debugfs_init(tvout, encoder->dev->primary);
+	tvout_debugfs_init(tvout, encoder);
 
 	tvout->debugfs_registered = true;
 	return 0;
-- 
2.39.0

[PATCH 12/13] drm/debugfs: Remove the debugfs late register function

[Maíra Canal]

With the new debugfs infrastructure for kms objects, there is no need to
have a late register helper for debugfs, as the late register debugfs
function was only created because drm_debugfs_init() is not able to
create debugfs files for modeset components, as they are registered after
the primary and the render drm_minor are registered.

Now that each kms object has its own debugfs init function, remove the
debugfs late register function.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/drm_debugfs.c     | 11 -----------
 drivers/gpu/drm/drm_internal.h    |  5 -----
 drivers/gpu/drm/drm_mode_config.c |  2 --
 3 files changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index e1f71a03a581..fe470a896527 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -294,17 +294,6 @@ void drm_debugfs_encoder_init(struct drm_encoder *encoder)
 	drm_create_file_from_list(encoder);
 }
 
-void drm_debugfs_late_register(struct drm_device *dev)
-{
-	struct drm_minor *minor = dev->primary;
-	struct drm_debugfs_entry *entry, *tmp;
-
-	if (!minor)
-		return;
-
-	drm_create_file_from_list(dev);
-}
-
 int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
 			     struct drm_minor *minor)
 {
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 9be697f7f8f9..eb6a0545249a 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -189,7 +189,6 @@ void drm_debugfs_crtc_init(struct drm_crtc *crtc);
 void drm_debugfs_connector_init(struct drm_connector *connector);
 void drm_debugfs_encoder_init(struct drm_encoder *encoder);
 void drm_debugfs_cleanup(struct drm_minor *minor);
-void drm_debugfs_late_register(struct drm_device *dev);
 void drm_debugfs_connector_add(struct drm_connector *connector);
 void drm_debugfs_connector_remove(struct drm_connector *connector);
 void drm_debugfs_crtc_add(struct drm_crtc *crtc);
@@ -218,10 +217,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor)
 {
 }
 
-static inline void drm_debugfs_late_register(struct drm_device *dev)
-{
-}
-
 static inline void drm_debugfs_connector_add(struct drm_connector *connector)
 {
 }
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 87eb591fe9b5..8525ef851540 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev)
 	if (ret)
 		goto err_connector;
 
-	drm_debugfs_late_register(dev);
-
 	return 0;
 
 err_connector:
-- 
2.39.0

[PATCH 13/13] drm/todo: Update the debugfs clean up task

[Maíra Canal]

The structs drm_debugfs_connector_entry, drm_debugfs_encoder_entry, and
drm_debugfs_crtc_entry introduced a debugfs infrastruture that made it
possible to remove late-register debugfs. So, update the debugfs clean
up task to include the advances for the kms objects and point out
possible improvements to be done.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 Documentation/gpu/todo.rst | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 1f8a5ebe188e..3c4d4b8541e8 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -511,14 +511,12 @@ There's a bunch of issues with it:
 - Convert drivers to support the drm_debugfs_add_files() function instead of
   the drm_debugfs_create_files() function.
 
-- Improve late-register debugfs by rolling out the same debugfs pre-register
-  infrastructure for connector and crtc too. That way, the drivers won't need to
-  split their setup code into init and register anymore.
-
-- We probably want to have some support for debugfs files on crtc/connectors and
-  maybe other kms objects directly in core. There's even drm_print support in
-  the funcs for these objects to dump kms state, so it's all there. And then the
-  ->show() functions should obviously give you a pointer to the right object.
+- Convert drivers to use the kms object debugfs helpers, such as
+  drm_debugfs_connector_add_files(), instead of the drm_debugfs_create_files()
+  function on late register hooks.
+
+- We probably want to have some support for debugfs files on planes, like we
+  have for crtc, connectors, and encoders.
 
 - The drm_driver->debugfs_init hooks we have is just an artifact of the old
   midlayered load sequence. DRM debugfs should work more like sysfs, where you
-- 
2.39.0