Re: [PATCH 1/3] drm/encoder: register per-encoder debugfs dir

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

On 04/09/2023 03:53, Dmitry Baryshkov wrote:
> Each of connectors and CRTCs used by the DRM device provides debugfs
> directory, which is used by several standard debugfs files and can
> further be extended by the driver. Add such generic debugfs directories
> for encoder.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/drm_debugfs.c  | 25 +++++++++++++++++++++++++
>   drivers/gpu/drm/drm_encoder.c  |  4 ++++
>   drivers/gpu/drm/drm_internal.h |  9 +++++++++
>   include/drm/drm_encoder.h      | 16 +++++++++++++++-
>   4 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 2de43ff3ce0a..cf7f33bdc963 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -603,4 +603,29 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
>   	crtc->debugfs_entry = NULL;
>   }
>   
> +void drm_debugfs_encoder_add(struct drm_encoder *encoder)
> +{
> +	struct drm_minor *minor = encoder->dev->primary;
> +	struct dentry *root;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "encoder-%d", encoder->index);
> +	if (!name)
> +		return;
> +
> +	root = debugfs_create_dir(name, minor->debugfs_root);
> +	kfree(name);
> +
> +	encoder->debugfs_entry = root;
> +
> +	if (encoder->funcs->debugfs_init)
> +		encoder->funcs->debugfs_init(encoder, root);
> +}
> +
> +void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
> +{
> +	debugfs_remove_recursive(encoder->debugfs_entry);
> +	encoder->debugfs_entry = NULL;
> +}
> +
>   #endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 1143bc7f3252..8f2bc6a28482 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -30,6 +30,7 @@
>   #include <drm/drm_print.h>
>   
>   #include "drm_crtc_internal.h"
> +#include "drm_internal.h"
>   
>   /**
>    * DOC: overview
> @@ -74,6 +75,8 @@ int drm_encoder_register_all(struct drm_device *dev)
>   	int ret = 0;
>   
>   	drm_for_each_encoder(encoder, dev) {
> +		drm_debugfs_encoder_add(encoder);
> +
>   		if (encoder->funcs && encoder->funcs->late_register)
>   			ret = encoder->funcs->late_register(encoder);
>   		if (ret)
> @@ -90,6 +93,7 @@ void drm_encoder_unregister_all(struct drm_device *dev)
>   	drm_for_each_encoder(encoder, dev) {
>   		if (encoder->funcs && encoder->funcs->early_unregister)
>   			encoder->funcs->early_unregister(encoder);
> +		drm_debugfs_encoder_remove(encoder);
>   	}
>   }
>   
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ba12acd55139..173b4d872431 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -189,6 +189,8 @@ void drm_debugfs_connector_remove(struct drm_connector *connector);
>   void drm_debugfs_crtc_add(struct drm_crtc *crtc);
>   void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
>   void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> +void drm_debugfs_encoder_add(struct drm_encoder *encoder);
> +void drm_debugfs_encoder_remove(struct drm_encoder *encoder);
>   #else
>   static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   				   struct dentry *root)
> @@ -222,6 +224,13 @@ static inline void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>   {
>   }
>   
> +static inline void drm_debugfs_encoder_add(struct drm_encoder *encoder)
> +{
> +}
> +static inline void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
> +{
> +}
> +
>   #endif
>   
>   drm_ioctl_t drm_version;
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 3a09682af685..977a9381c8ba 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -60,7 +60,7 @@ struct drm_encoder_funcs {
>   	 * @late_register:
>   	 *
>   	 * This optional hook can be used to register additional userspace
> -	 * interfaces attached to the encoder like debugfs interfaces.
> +	 * interfaces attached to the encoder.
>   	 * It is called late in the driver load sequence from drm_dev_register().
>   	 * Everything added from this callback should be unregistered in
>   	 * the early_unregister callback.
> @@ -81,6 +81,13 @@ struct drm_encoder_funcs {
>   	 * before data structures are torndown.
>   	 */
>   	void (*early_unregister)(struct drm_encoder *encoder);
> +
> +	/**
> +	 * @debugfs_init:
> +	 *
> +	 * Allows encoders to create encoder-specific debugfs files.
> +	 */
> +	void (*debugfs_init)(struct drm_encoder *encoder, struct dentry *root);
>   };
>   
>   /**
> @@ -184,6 +191,13 @@ struct drm_encoder {
>   
>   	const struct drm_encoder_funcs *funcs;
>   	const struct drm_encoder_helper_funcs *helper_private;
> +
> +	/**
> +	 * @debugfs_entry:
> +	 *
> +	 * Debugfs directory for this CRTC.
> +	 */
> +	struct dentry *debugfs_entry;
>   };
>   
>   #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)

Looks fine:
Reviewed-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>

I'll need a core ack to apply to drm-misc with patch 2

Neil