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

[Jani Nikula <jani.nikula@linux.intel.com>]

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