[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 at 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 at 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