On Thu, Jan 12, 2023 at 11:19:37AM +0200, Jani Nikula wrote:
> On Wed, 11 Jan 2023, Maíra Canal <mcanal@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
