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.
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
