On Thu, Feb 06, 2025 at 07:14:20PM +0100, Luca Ceresoli wrote:
> The global bridges_list holding all the bridges between drm_bridge_add()
> and drm_bridge_remove() cannot be inspected via debugfs. Add a file showing
> it.
>
> To avoid code duplication, move the code printing a bridge info to a common
> function.
>
> Note: this change requires exporting bridge_list and the mutex protecting
> it.
>
> Also add a comment about bridge_lock to make checkpatch happy.
I think, exporting mutex _and_ a list is a bad idea (especially since
they don't have a proper prefix). It might be better to make the
bridge_print() function a more public one (and name it
drm_bridge_print()) and move allbridges attribute definition to
drm_bridge.c
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
>
> ---
>
> This patch was added in v6.
> ---
> drivers/gpu/drm/drm_bridge.c | 5 +--
> drivers/gpu/drm/drm_debugfs.c | 70 +++++++++++++++++++++++++++++-------------
> drivers/gpu/drm/drm_drv.c | 1 +
> drivers/gpu/drm/drm_internal.h | 9 ++++++
> 4 files changed, 61 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 241a384ebce39b4a3db58c208af27960904fc662..87cebec2de806781cee22da54d666eee9bde3648 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -195,8 +195,9 @@
> * driver.
> */
>
> -static DEFINE_MUTEX(bridge_lock);
> -static LIST_HEAD(bridge_list);
> +/* Protect bridge_list */
> +DEFINE_MUTEX(bridge_lock);
> +LIST_HEAD(bridge_list);
>
> /**
> * drm_bridge_add - add the given bridge to the global bridge list
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b2178864c7ee12db9aa1f562e106b2f604439f8..7424d5237e7615d63de6bba572ee6050da6709d0 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -740,6 +740,30 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> crtc->debugfs_entry = NULL;
> }
>
> +static void bridge_print(struct drm_printer *p, struct drm_bridge *bridge, unsigned int idx)
> +{
> + drm_printf(p, "bridge[%u]: %ps\n", idx, bridge->funcs);
> + drm_printf(p, "\ttype: [%d] %s\n",
> + bridge->type,
> + drm_get_connector_type_name(bridge->type));
> +
> + if (bridge->of_node)
> + drm_printf(p, "\tOF: %pOFfc\n", bridge->of_node);
> +
> + drm_printf(p, "\tops: [0x%x]", bridge->ops);
> + if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> + drm_puts(p, " detect");
> + if (bridge->ops & DRM_BRIDGE_OP_EDID)
> + drm_puts(p, " edid");
> + if (bridge->ops & DRM_BRIDGE_OP_HPD)
> + drm_puts(p, " hpd");
> + if (bridge->ops & DRM_BRIDGE_OP_MODES)
> + drm_puts(p, " modes");
> + if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> + drm_puts(p, " hdmi");
> + drm_puts(p, "\n");
> +}
> +
> static int bridges_show(struct seq_file *m, void *data)
> {
> struct drm_encoder *encoder = m->private;
> @@ -747,28 +771,8 @@ static int bridges_show(struct seq_file *m, void *data)
> struct drm_bridge *bridge;
> unsigned int idx = 0;
>
> - drm_for_each_bridge_in_chain(encoder, bridge) {
> - drm_printf(&p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
> - drm_printf(&p, "\ttype: [%d] %s\n",
> - bridge->type,
> - drm_get_connector_type_name(bridge->type));
> -
> - if (bridge->of_node)
> - drm_printf(&p, "\tOF: %pOFfc\n", bridge->of_node);
> -
> - drm_printf(&p, "\tops: [0x%x]", bridge->ops);
> - if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> - drm_puts(&p, " detect");
> - if (bridge->ops & DRM_BRIDGE_OP_EDID)
> - drm_puts(&p, " edid");
> - if (bridge->ops & DRM_BRIDGE_OP_HPD)
> - drm_puts(&p, " hpd");
> - if (bridge->ops & DRM_BRIDGE_OP_MODES)
> - drm_puts(&p, " modes");
> - if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> - drm_puts(&p, " hdmi");
> - drm_puts(&p, "\n");
> - }
> + drm_for_each_bridge_in_chain(encoder, bridge)
> + bridge_print(&p, bridge, idx++);
>
> return 0;
> }
> @@ -802,3 +806,25 @@ void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
> debugfs_remove_recursive(encoder->debugfs_entry);
> encoder->debugfs_entry = NULL;
> }
> +
> +static int allbridges_show(struct seq_file *m, void *data)
> +{
> + struct drm_printer p = drm_seq_file_printer(m);
> + struct drm_bridge *bridge;
> + unsigned int idx = 0;
> +
> + mutex_lock(&bridge_lock);
> +
> + list_for_each_entry(bridge, &bridge_list, list)
> + bridge_print(&p, bridge, idx++);
> +
> + mutex_unlock(&bridge_lock);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(allbridges);
Should it be DEFINE_DEBUGFS_ATTRIBUTE instead?
> +
> +void drm_debugfs_global_add(struct dentry *root)
> +{
> + debugfs_create_file("bridges", 0444, root, NULL, &allbridges_fops);
> +}
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 3cf440eee8a2ab3de134d925db8f1d2ce68062b7..9b6d7bd16ba409b6a9155a9fecbec6bfdd5ea0c2 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1120,6 +1120,7 @@ static int __init drm_core_init(void)
> }
>
> drm_debugfs_root = debugfs_create_dir("dri", NULL);
> + drm_debugfs_global_add(drm_debugfs_root);
>
> ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
> if (ret < 0)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b2b6a8e49dda46f1cb3b048ef7b28356dd3aaa4e..b6e875d4b25faae6bb0bb952c3c12bd4819698ec 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -48,6 +48,10 @@ struct drm_prime_file_private;
> struct drm_printer;
> struct drm_vblank_crtc;
>
> +// for drm_debugfs.c
> +extern struct mutex bridge_lock;
> +extern struct list_head bridge_list;
> +
> /* drm_client_event.c */
> #if defined(CONFIG_DRM_CLIENT)
> void drm_client_debugfs_init(struct drm_device *dev);
> @@ -196,6 +200,7 @@ 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);
> +void drm_debugfs_global_add(struct dentry *drm_debugfs_root);
> #else
> static inline void drm_debugfs_dev_fini(struct drm_device *dev)
> {
> @@ -241,6 +246,10 @@ static inline void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
> {
> }
>
> +static inline void drm_debugfs_global_add(struct dentry *drm_debugfs_root)
> +{
> +}
> +
> #endif
>
> drm_ioctl_t drm_version;
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
