| Message ID | 20230111173748.752659-8-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/debugfs: Create a debugfs infrastructure for kms objects | expand |
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; > }
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
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; }
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(-)