[PATCH 0/3] drm: introduce per-encoder debugfs directory

[PATCH 0/3] drm: introduce per-encoder debugfs directory

[Dmitry Baryshkov]

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. As a showcase for this dir, migrate `bridge_chains' debugfs
file (which contains per-encoder data) and MSM custom encoder status to
this new debugfs directory.

Dmitry Baryshkov (3):
  drm/encoder: register per-encoder debugfs dir
  drm/bridge: migrate bridge_chains to per-encoder file
  drm/msm/dpu: move encoder status to standard encoder debugfs dir

 drivers/gpu/drm/drm_bridge.c                | 44 --------------
 drivers/gpu/drm/drm_debugfs.c               | 64 ++++++++++++++++++++-
 drivers/gpu/drm/drm_encoder.c               |  4 ++
 drivers/gpu/drm/drm_internal.h              |  9 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 ++-------------
 include/drm/drm_bridge.h                    |  2 -
 include/drm/drm_encoder.h                   | 16 +++++-
 7 files changed, 95 insertions(+), 89 deletions(-)

-- 
2.39.2

[PATCH 0/3] drm: introduce per-encoder debugfs directory

[Dmitry Baryshkov]

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. As a showcase for this dir, migrate `bridge_chains' debugfs
file (which contains per-encoder data) and MSM custom encoder status to
this new debugfs directory.

Dmitry Baryshkov (3):
  drm/encoder: register per-encoder debugfs dir
  drm/bridge: migrate bridge_chains to per-encoder file
  drm/msm/dpu: move encoder status to standard encoder debugfs dir

 drivers/gpu/drm/drm_bridge.c                | 44 --------------
 drivers/gpu/drm/drm_debugfs.c               | 64 ++++++++++++++++++++-
 drivers/gpu/drm/drm_encoder.c               |  4 ++
 drivers/gpu/drm/drm_internal.h              |  9 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 ++-------------
 include/drm/drm_bridge.h                    |  2 -
 include/drm/drm_encoder.h                   | 16 +++++-
 7 files changed, 95 insertions(+), 89 deletions(-)

-- 
2.39.2

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

[Dmitry Baryshkov]

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@linaro.org>
---
 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)
-- 
2.39.2

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

[Dmitry Baryshkov]

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@linaro.org>
---
 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)
-- 
2.39.2

[PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file

[Dmitry Baryshkov]

Instead of having a single file with all bridge chains, list bridges
under a corresponding per-encoder debugfs directory.

Example of the listing:

$ cat /sys/kernel/debug/dri/0/encoder-0/bridges
bridge[0]: dsi_mgr_bridge_funcs
	type: [0] Unknown
	ops: [0]
bridge[1]: lt9611uxc_bridge_funcs
	type: [11] HDMI-A
	OF: /soc@0/geniqup@9c0000/i2c@994000/hdmi-bridge@2b:lontium,lt9611uxc
	ops: [7] detect edid hpd

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
 drivers/gpu/drm/drm_debugfs.c | 39 ++++++++++++++++++++++++++++---
 include/drm/drm_bridge.h      |  2 --
 3 files changed, 36 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 39e68e45bb12..cee3188adf3d 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1347,50 +1347,6 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 EXPORT_SYMBOL(of_drm_find_bridge);
 #endif
 
-#ifdef CONFIG_DEBUG_FS
-static int drm_bridge_chains_info(struct seq_file *m, void *data)
-{
-	struct drm_debugfs_entry *entry = m->private;
-	struct drm_device *dev = entry->dev;
-	struct drm_printer p = drm_seq_file_printer(m);
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_encoder *encoder;
-	unsigned int bridge_idx = 0;
-
-	list_for_each_entry(encoder, &config->encoder_list, head) {
-		struct drm_bridge *bridge;
-
-		drm_printf(&p, "encoder[%u]\n", encoder->base.id);
-
-		drm_for_each_bridge_in_chain(encoder, bridge) {
-			drm_printf(&p, "\tbridge[%u] type: %u, ops: %#x",
-				   bridge_idx, bridge->type, bridge->ops);
-
-#ifdef CONFIG_OF
-			if (bridge->of_node)
-				drm_printf(&p, ", OF: %pOFfc", bridge->of_node);
-#endif
-
-			drm_printf(&p, "\n");
-
-			bridge_idx++;
-		}
-	}
-
-	return 0;
-}
-
-static const struct drm_debugfs_info drm_bridge_debugfs_list[] = {
-	{ "bridge_chains", drm_bridge_chains_info, 0 },
-};
-
-void drm_bridge_debugfs_init(struct drm_minor *minor)
-{
-	drm_debugfs_add_files(minor->dev, drm_bridge_debugfs_list,
-			      ARRAY_SIZE(drm_bridge_debugfs_list));
-}
-#endif
-
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
 MODULE_DESCRIPTION("DRM bridge infrastructure");
 MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index cf7f33bdc963..70913067406d 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -273,10 +273,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 
 	drm_debugfs_add_files(minor->dev, drm_debugfs_list, DRM_DEBUGFS_ENTRIES);
 
-	if (drm_drv_uses_atomic_modeset(dev)) {
+	if (drm_drv_uses_atomic_modeset(dev))
 		drm_atomic_debugfs_init(minor);
-		drm_bridge_debugfs_init(minor);
-	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_framebuffer_debugfs_init(minor);
@@ -603,6 +601,37 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
 	crtc->debugfs_entry = NULL;
 }
 
+static int bridges_show(struct seq_file *m, void *data)
+{
+	struct drm_encoder *encoder = m->private;
+	struct drm_bridge *bridge;
+	unsigned int idx = 0;
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		seq_printf(m, "bridge[%d]: %ps\n", idx++, bridge->funcs);
+		seq_printf(m, "\ttype: [%d] %s\n",
+			   bridge->type,
+			   drm_get_connector_type_name(bridge->type));
+#ifdef CONFIG_OF
+		if (bridge->of_node)
+			seq_printf(m, "\tOF: %pOFfc\n", bridge->of_node);
+#endif
+		seq_printf(m, "\tops: [0x%x]", bridge->ops);
+		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
+			seq_puts(m, " detect");
+		if (bridge->ops & DRM_BRIDGE_OP_EDID)
+			seq_puts(m, " edid");
+		if (bridge->ops & DRM_BRIDGE_OP_HPD)
+			seq_puts(m, " hpd");
+		if (bridge->ops & DRM_BRIDGE_OP_MODES)
+			seq_puts(m, " modes");
+		seq_puts(m, "\n");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(bridges);
+
 void drm_debugfs_encoder_add(struct drm_encoder *encoder)
 {
 	struct drm_minor *minor = encoder->dev->primary;
@@ -618,6 +647,10 @@ void drm_debugfs_encoder_add(struct drm_encoder *encoder)
 
 	encoder->debugfs_entry = root;
 
+	/* bridges list */
+	debugfs_create_file("bridges", 0444, root, encoder,
+			    &bridges_fops);
+
 	if (encoder->funcs->debugfs_init)
 		encoder->funcs->debugfs_init(encoder, root);
 }
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c339fc85fd07..902bc3f99c2a 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -950,6 +950,4 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
 }
 #endif
 
-void drm_bridge_debugfs_init(struct drm_minor *minor);
-
 #endif
-- 
2.39.2

[PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file

[Dmitry Baryshkov]

Instead of having a single file with all bridge chains, list bridges
under a corresponding per-encoder debugfs directory.

Example of the listing:

$ cat /sys/kernel/debug/dri/0/encoder-0/bridges
bridge[0]: dsi_mgr_bridge_funcs
	type: [0] Unknown
	ops: [0]
bridge[1]: lt9611uxc_bridge_funcs
	type: [11] HDMI-A
	OF: /soc@0/geniqup@9c0000/i2c@994000/hdmi-bridge@2b:lontium,lt9611uxc
	ops: [7] detect edid hpd

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
 drivers/gpu/drm/drm_debugfs.c | 39 ++++++++++++++++++++++++++++---
 include/drm/drm_bridge.h      |  2 --
 3 files changed, 36 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 39e68e45bb12..cee3188adf3d 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1347,50 +1347,6 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 EXPORT_SYMBOL(of_drm_find_bridge);
 #endif
 
-#ifdef CONFIG_DEBUG_FS
-static int drm_bridge_chains_info(struct seq_file *m, void *data)
-{
-	struct drm_debugfs_entry *entry = m->private;
-	struct drm_device *dev = entry->dev;
-	struct drm_printer p = drm_seq_file_printer(m);
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_encoder *encoder;
-	unsigned int bridge_idx = 0;
-
-	list_for_each_entry(encoder, &config->encoder_list, head) {
-		struct drm_bridge *bridge;
-
-		drm_printf(&p, "encoder[%u]\n", encoder->base.id);
-
-		drm_for_each_bridge_in_chain(encoder, bridge) {
-			drm_printf(&p, "\tbridge[%u] type: %u, ops: %#x",
-				   bridge_idx, bridge->type, bridge->ops);
-
-#ifdef CONFIG_OF
-			if (bridge->of_node)
-				drm_printf(&p, ", OF: %pOFfc", bridge->of_node);
-#endif
-
-			drm_printf(&p, "\n");
-
-			bridge_idx++;
-		}
-	}
-
-	return 0;
-}
-
-static const struct drm_debugfs_info drm_bridge_debugfs_list[] = {
-	{ "bridge_chains", drm_bridge_chains_info, 0 },
-};
-
-void drm_bridge_debugfs_init(struct drm_minor *minor)
-{
-	drm_debugfs_add_files(minor->dev, drm_bridge_debugfs_list,
-			      ARRAY_SIZE(drm_bridge_debugfs_list));
-}
-#endif
-
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
 MODULE_DESCRIPTION("DRM bridge infrastructure");
 MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index cf7f33bdc963..70913067406d 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -273,10 +273,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 
 	drm_debugfs_add_files(minor->dev, drm_debugfs_list, DRM_DEBUGFS_ENTRIES);
 
-	if (drm_drv_uses_atomic_modeset(dev)) {
+	if (drm_drv_uses_atomic_modeset(dev))
 		drm_atomic_debugfs_init(minor);
-		drm_bridge_debugfs_init(minor);
-	}
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_framebuffer_debugfs_init(minor);
@@ -603,6 +601,37 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
 	crtc->debugfs_entry = NULL;
 }
 
+static int bridges_show(struct seq_file *m, void *data)
+{
+	struct drm_encoder *encoder = m->private;
+	struct drm_bridge *bridge;
+	unsigned int idx = 0;
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		seq_printf(m, "bridge[%d]: %ps\n", idx++, bridge->funcs);
+		seq_printf(m, "\ttype: [%d] %s\n",
+			   bridge->type,
+			   drm_get_connector_type_name(bridge->type));
+#ifdef CONFIG_OF
+		if (bridge->of_node)
+			seq_printf(m, "\tOF: %pOFfc\n", bridge->of_node);
+#endif
+		seq_printf(m, "\tops: [0x%x]", bridge->ops);
+		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
+			seq_puts(m, " detect");
+		if (bridge->ops & DRM_BRIDGE_OP_EDID)
+			seq_puts(m, " edid");
+		if (bridge->ops & DRM_BRIDGE_OP_HPD)
+			seq_puts(m, " hpd");
+		if (bridge->ops & DRM_BRIDGE_OP_MODES)
+			seq_puts(m, " modes");
+		seq_puts(m, "\n");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(bridges);
+
 void drm_debugfs_encoder_add(struct drm_encoder *encoder)
 {
 	struct drm_minor *minor = encoder->dev->primary;
@@ -618,6 +647,10 @@ void drm_debugfs_encoder_add(struct drm_encoder *encoder)
 
 	encoder->debugfs_entry = root;
 
+	/* bridges list */
+	debugfs_create_file("bridges", 0444, root, encoder,
+			    &bridges_fops);
+
 	if (encoder->funcs->debugfs_init)
 		encoder->funcs->debugfs_init(encoder, root);
 }
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c339fc85fd07..902bc3f99c2a 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -950,6 +950,4 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
 }
 #endif
 
-void drm_bridge_debugfs_init(struct drm_minor *minor);
-
 #endif
-- 
2.39.2

[PATCH 3/3] drm/msm/dpu: move encoder status to standard encoder debugfs dir

[Dmitry Baryshkov]

Now as we have standard per-encoder debugfs directory, move DPU encoder
status file to that directory.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d34e684a4178..b219382d1153 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -184,7 +184,6 @@ struct dpu_encoder_virt {
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
 
-	struct dentry *debugfs_root;
 	struct mutex enc_lock;
 	DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
 	void (*crtc_frame_event_cb)(void *, u32 event);
@@ -2096,7 +2095,8 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 #ifdef CONFIG_DEBUG_FS
 static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 {
-	struct dpu_encoder_virt *dpu_enc = s->private;
+	struct drm_encoder *drm_enc = s->private;
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
 	int i;
 
 	mutex_lock(&dpu_enc->enc_lock);
@@ -2118,48 +2118,16 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
 
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void dpu_encoder_debugfs_init(struct drm_encoder *drm_enc, struct dentry *root)
 {
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-
-	char name[12];
-
-	if (!drm_enc->dev) {
-		DPU_ERROR("invalid encoder or kms\n");
-		return -EINVAL;
-	}
-
-	snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
-
-	/* create overall sub-directory for the encoder */
-	dpu_enc->debugfs_root = debugfs_create_dir(name,
-			drm_enc->dev->primary->debugfs_root);
-
 	/* don't error check these */
 	debugfs_create_file("status", 0600,
-		dpu_enc->debugfs_root, dpu_enc, &_dpu_encoder_status_fops);
-
-	return 0;
+			    root, drm_enc, &_dpu_encoder_status_fops);
 }
 #else
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
-{
-	return 0;
-}
+#define dpu_encoder_debugfs_init NULL
 #endif
 
-static int dpu_encoder_late_register(struct drm_encoder *encoder)
-{
-	return _dpu_encoder_init_debugfs(encoder);
-}
-
-static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
-{
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
-
-	debugfs_remove_recursive(dpu_enc->debugfs_root);
-}
-
 static int dpu_encoder_virt_add_phys_encs(
 		struct msm_display_info *disp_info,
 		struct dpu_encoder_virt *dpu_enc,
@@ -2343,8 +2311,7 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
 
 static const struct drm_encoder_funcs dpu_encoder_funcs = {
 		.destroy = dpu_encoder_destroy,
-		.late_register = dpu_encoder_late_register,
-		.early_unregister = dpu_encoder_early_unregister,
+		.debugfs_init = dpu_encoder_debugfs_init,
 };
 
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
-- 
2.39.2

[PATCH 3/3] drm/msm/dpu: move encoder status to standard encoder debugfs dir

[Dmitry Baryshkov]

Now as we have standard per-encoder debugfs directory, move DPU encoder
status file to that directory.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d34e684a4178..b219382d1153 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -184,7 +184,6 @@ struct dpu_encoder_virt {
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
 
-	struct dentry *debugfs_root;
 	struct mutex enc_lock;
 	DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
 	void (*crtc_frame_event_cb)(void *, u32 event);
@@ -2096,7 +2095,8 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 #ifdef CONFIG_DEBUG_FS
 static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 {
-	struct dpu_encoder_virt *dpu_enc = s->private;
+	struct drm_encoder *drm_enc = s->private;
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
 	int i;
 
 	mutex_lock(&dpu_enc->enc_lock);
@@ -2118,48 +2118,16 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
 
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void dpu_encoder_debugfs_init(struct drm_encoder *drm_enc, struct dentry *root)
 {
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-
-	char name[12];
-
-	if (!drm_enc->dev) {
-		DPU_ERROR("invalid encoder or kms\n");
-		return -EINVAL;
-	}
-
-	snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
-
-	/* create overall sub-directory for the encoder */
-	dpu_enc->debugfs_root = debugfs_create_dir(name,
-			drm_enc->dev->primary->debugfs_root);
-
 	/* don't error check these */
 	debugfs_create_file("status", 0600,
-		dpu_enc->debugfs_root, dpu_enc, &_dpu_encoder_status_fops);
-
-	return 0;
+			    root, drm_enc, &_dpu_encoder_status_fops);
 }
 #else
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
-{
-	return 0;
-}
+#define dpu_encoder_debugfs_init NULL
 #endif
 
-static int dpu_encoder_late_register(struct drm_encoder *encoder)
-{
-	return _dpu_encoder_init_debugfs(encoder);
-}
-
-static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
-{
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
-
-	debugfs_remove_recursive(dpu_enc->debugfs_root);
-}
-
 static int dpu_encoder_virt_add_phys_encs(
 		struct msm_display_info *disp_info,
 		struct dpu_encoder_virt *dpu_enc,
@@ -2343,8 +2311,7 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
 
 static const struct drm_encoder_funcs dpu_encoder_funcs = {
 		.destroy = dpu_encoder_destroy,
-		.late_register = dpu_encoder_late_register,
-		.early_unregister = dpu_encoder_early_unregister,
+		.debugfs_init = dpu_encoder_debugfs_init,
 };
 
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
-- 
2.39.2

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

[Neil Armstrong]

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@linaro.org>
> ---
>   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@linaro.org>

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

Neil

Re: [PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file

[Neil Armstrong]

Hi,

On 04/09/2023 03:53, Dmitry Baryshkov wrote:
> Instead of having a single file with all bridge chains, list bridges
> under a corresponding per-encoder debugfs directory.
> 
> Example of the listing:
> 
> $ cat /sys/kernel/debug/dri/0/encoder-0/bridges
> bridge[0]: dsi_mgr_bridge_funcs
> 	type: [0] Unknown
> 	ops: [0]
> bridge[1]: lt9611uxc_bridge_funcs
> 	type: [11] HDMI-A
> 	OF: /soc@0/geniqup@9c0000/i2c@994000/hdmi-bridge@2b:lontium,lt9611uxc
> 	ops: [7] detect edid hpd
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
>   drivers/gpu/drm/drm_debugfs.c | 39 ++++++++++++++++++++++++++++---
>   include/drm/drm_bridge.h      |  2 --
>   3 files changed, 36 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 39e68e45bb12..cee3188adf3d 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1347,50 +1347,6 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>   EXPORT_SYMBOL(of_drm_find_bridge);
>   #endif
>   
> -#ifdef CONFIG_DEBUG_FS
> -static int drm_bridge_chains_info(struct seq_file *m, void *data)
> -{
> -	struct drm_debugfs_entry *entry = m->private;
> -	struct drm_device *dev = entry->dev;
> -	struct drm_printer p = drm_seq_file_printer(m);
> -	struct drm_mode_config *config = &dev->mode_config;
> -	struct drm_encoder *encoder;
> -	unsigned int bridge_idx = 0;
> -
> -	list_for_each_entry(encoder, &config->encoder_list, head) {
> -		struct drm_bridge *bridge;
> -
> -		drm_printf(&p, "encoder[%u]\n", encoder->base.id);
> -
> -		drm_for_each_bridge_in_chain(encoder, bridge) {
> -			drm_printf(&p, "\tbridge[%u] type: %u, ops: %#x",
> -				   bridge_idx, bridge->type, bridge->ops);
> -
> -#ifdef CONFIG_OF
> -			if (bridge->of_node)
> -				drm_printf(&p, ", OF: %pOFfc", bridge->of_node);
> -#endif
> -
> -			drm_printf(&p, "\n");
> -
> -			bridge_idx++;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static const struct drm_debugfs_info drm_bridge_debugfs_list[] = {
> -	{ "bridge_chains", drm_bridge_chains_info, 0 },
> -};
> -
> -void drm_bridge_debugfs_init(struct drm_minor *minor)
> -{
> -	drm_debugfs_add_files(minor->dev, drm_bridge_debugfs_list,
> -			      ARRAY_SIZE(drm_bridge_debugfs_list));
> -}
> -#endif
> -
>   MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
>   MODULE_DESCRIPTION("DRM bridge infrastructure");
>   MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index cf7f33bdc963..70913067406d 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -273,10 +273,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   
>   	drm_debugfs_add_files(minor->dev, drm_debugfs_list, DRM_DEBUGFS_ENTRIES);
>   
> -	if (drm_drv_uses_atomic_modeset(dev)) {
> +	if (drm_drv_uses_atomic_modeset(dev))
>   		drm_atomic_debugfs_init(minor);
> -		drm_bridge_debugfs_init(minor);
> -	}
>   
>   	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   		drm_framebuffer_debugfs_init(minor);
> @@ -603,6 +601,37 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
>   	crtc->debugfs_entry = NULL;
>   }
>   
> +static int bridges_show(struct seq_file *m, void *data)
> +{
> +	struct drm_encoder *encoder = m->private;
> +	struct drm_bridge *bridge;
> +	unsigned int idx = 0;
> +
> +	drm_for_each_bridge_in_chain(encoder, bridge) {
> +		seq_printf(m, "bridge[%d]: %ps\n", idx++, bridge->funcs);
> +		seq_printf(m, "\ttype: [%d] %s\n",
> +			   bridge->type,
> +			   drm_get_connector_type_name(bridge->type));
> +#ifdef CONFIG_OF
> +		if (bridge->of_node)
> +			seq_printf(m, "\tOF: %pOFfc\n", bridge->of_node);
> +#endif
> +		seq_printf(m, "\tops: [0x%x]", bridge->ops);
> +		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> +			seq_puts(m, " detect");
> +		if (bridge->ops & DRM_BRIDGE_OP_EDID)
> +			seq_puts(m, " edid");
> +		if (bridge->ops & DRM_BRIDGE_OP_HPD)
> +			seq_puts(m, " hpd");
> +		if (bridge->ops & DRM_BRIDGE_OP_MODES)
> +			seq_puts(m, " modes");
> +		seq_puts(m, "\n");
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(bridges);
> +
>   void drm_debugfs_encoder_add(struct drm_encoder *encoder)
>   {
>   	struct drm_minor *minor = encoder->dev->primary;
> @@ -618,6 +647,10 @@ void drm_debugfs_encoder_add(struct drm_encoder *encoder)
>   
>   	encoder->debugfs_entry = root;
>   
> +	/* bridges list */
> +	debugfs_create_file("bridges", 0444, root, encoder,
> +			    &bridges_fops);
> +
>   	if (encoder->funcs->debugfs_init)
>   		encoder->funcs->debugfs_init(encoder, root);
>   }
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index c339fc85fd07..902bc3f99c2a 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -950,6 +950,4 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
>   }
>   #endif
>   
> -void drm_bridge_debugfs_init(struct drm_minor *minor);
> -
>   #endif

It would be nice to have a review from Tomi since he pushed the bridge chains debugfs.

Apart that it looks fine:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

Re: [PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file

[Tomi Valkeinen]

Hi,

On 06/10/2023 10:35, Neil Armstrong wrote:
> Hi,
> 
> On 04/09/2023 03:53, Dmitry Baryshkov wrote:
>> Instead of having a single file with all bridge chains, list bridges
>> under a corresponding per-encoder debugfs directory.
>>
>> Example of the listing:
>>
>> $ cat /sys/kernel/debug/dri/0/encoder-0/bridges
>> bridge[0]: dsi_mgr_bridge_funcs
>>     type: [0] Unknown
>>     ops: [0]
>> bridge[1]: lt9611uxc_bridge_funcs
>>     type: [11] HDMI-A
>>     OF: /soc@0/geniqup@9c0000/i2c@994000/hdmi-bridge@2b:lontium,lt9611uxc
>>     ops: [7] detect edid hpd
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
>>   drivers/gpu/drm/drm_debugfs.c | 39 ++++++++++++++++++++++++++++---
>>   include/drm/drm_bridge.h      |  2 --
>>   3 files changed, 36 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 39e68e45bb12..cee3188adf3d 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -1347,50 +1347,6 @@ struct drm_bridge *of_drm_find_bridge(struct 
>> device_node *np)
>>   EXPORT_SYMBOL(of_drm_find_bridge);
>>   #endif
>> -#ifdef CONFIG_DEBUG_FS
>> -static int drm_bridge_chains_info(struct seq_file *m, void *data)
>> -{
>> -    struct drm_debugfs_entry *entry = m->private;
>> -    struct drm_device *dev = entry->dev;
>> -    struct drm_printer p = drm_seq_file_printer(m);
>> -    struct drm_mode_config *config = &dev->mode_config;
>> -    struct drm_encoder *encoder;
>> -    unsigned int bridge_idx = 0;
>> -
>> -    list_for_each_entry(encoder, &config->encoder_list, head) {
>> -        struct drm_bridge *bridge;
>> -
>> -        drm_printf(&p, "encoder[%u]\n", encoder->base.id);
>> -
>> -        drm_for_each_bridge_in_chain(encoder, bridge) {
>> -            drm_printf(&p, "\tbridge[%u] type: %u, ops: %#x",
>> -                   bridge_idx, bridge->type, bridge->ops);
>> -
>> -#ifdef CONFIG_OF
>> -            if (bridge->of_node)
>> -                drm_printf(&p, ", OF: %pOFfc", bridge->of_node);
>> -#endif
>> -
>> -            drm_printf(&p, "\n");
>> -
>> -            bridge_idx++;
>> -        }
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>> -static const struct drm_debugfs_info drm_bridge_debugfs_list[] = {
>> -    { "bridge_chains", drm_bridge_chains_info, 0 },
>> -};
>> -
>> -void drm_bridge_debugfs_init(struct drm_minor *minor)
>> -{
>> -    drm_debugfs_add_files(minor->dev, drm_bridge_debugfs_list,
>> -                  ARRAY_SIZE(drm_bridge_debugfs_list));
>> -}
>> -#endif
>> -
>>   MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
>>   MODULE_DESCRIPTION("DRM bridge infrastructure");
>>   MODULE_LICENSE("GPL and additional rights");
>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>> b/drivers/gpu/drm/drm_debugfs.c
>> index cf7f33bdc963..70913067406d 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -273,10 +273,8 @@ int drm_debugfs_init(struct drm_minor *minor, int 
>> minor_id,
>>       drm_debugfs_add_files(minor->dev, drm_debugfs_list, 
>> DRM_DEBUGFS_ENTRIES);
>> -    if (drm_drv_uses_atomic_modeset(dev)) {
>> +    if (drm_drv_uses_atomic_modeset(dev))
>>           drm_atomic_debugfs_init(minor);
>> -        drm_bridge_debugfs_init(minor);
>> -    }
>>       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>           drm_framebuffer_debugfs_init(minor);
>> @@ -603,6 +601,37 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
>>       crtc->debugfs_entry = NULL;
>>   }
>> +static int bridges_show(struct seq_file *m, void *data)
>> +{
>> +    struct drm_encoder *encoder = m->private;
>> +    struct drm_bridge *bridge;
>> +    unsigned int idx = 0;
>> +
>> +    drm_for_each_bridge_in_chain(encoder, bridge) {
>> +        seq_printf(m, "bridge[%d]: %ps\n", idx++, bridge->funcs);
>> +        seq_printf(m, "\ttype: [%d] %s\n",
>> +               bridge->type,
>> +               drm_get_connector_type_name(bridge->type));
>> +#ifdef CONFIG_OF
>> +        if (bridge->of_node)
>> +            seq_printf(m, "\tOF: %pOFfc\n", bridge->of_node);
>> +#endif
>> +        seq_printf(m, "\tops: [0x%x]", bridge->ops);
>> +        if (bridge->ops & DRM_BRIDGE_OP_DETECT)
>> +            seq_puts(m, " detect");
>> +        if (bridge->ops & DRM_BRIDGE_OP_EDID)
>> +            seq_puts(m, " edid");
>> +        if (bridge->ops & DRM_BRIDGE_OP_HPD)
>> +            seq_puts(m, " hpd");
>> +        if (bridge->ops & DRM_BRIDGE_OP_MODES)
>> +            seq_puts(m, " modes");
>> +        seq_puts(m, "\n");
>> +    }
>> +
>> +    return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(bridges);
>> +
>>   void drm_debugfs_encoder_add(struct drm_encoder *encoder)
>>   {
>>       struct drm_minor *minor = encoder->dev->primary;
>> @@ -618,6 +647,10 @@ void drm_debugfs_encoder_add(struct drm_encoder 
>> *encoder)
>>       encoder->debugfs_entry = root;
>> +    /* bridges list */
>> +    debugfs_create_file("bridges", 0444, root, encoder,
>> +                &bridges_fops);
>> +
>>       if (encoder->funcs->debugfs_init)
>>           encoder->funcs->debugfs_init(encoder, root);
>>   }
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index c339fc85fd07..902bc3f99c2a 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -950,6 +950,4 @@ static inline struct drm_bridge 
>> *drmm_of_get_bridge(struct drm_device *drm,
>>   }
>>   #endif
>> -void drm_bridge_debugfs_init(struct drm_minor *minor);
>> -
>>   #endif
> 
> It would be nice to have a review from Tomi since he pushed the bridge 
> chains debugfs.
> 
> Apart that it looks fine:
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

This change does more than move the code to per-encoder debugfs file: it 
changes the formatting, adding textual representations for the flags, 
and drops the use of drm_printer.

I'd prefer to have such changes separately, but as it's a small patch I 
guess it's fine-ish. But they should at least be mentioned in the patch 
description.

With that addressed:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file

[Dmitry Baryshkov]

On 06/10/2023 10:51, Tomi Valkeinen wrote:
> Hi,
> 
> On 06/10/2023 10:35, Neil Armstrong wrote:
>> Hi,
>>
>> On 04/09/2023 03:53, Dmitry Baryshkov wrote:
>>> Instead of having a single file with all bridge chains, list bridges
>>> under a corresponding per-encoder debugfs directory.
>>>
>>> Example of the listing:
>>>
>>> $ cat /sys/kernel/debug/dri/0/encoder-0/bridges
>>> bridge[0]: dsi_mgr_bridge_funcs
>>>     type: [0] Unknown
>>>     ops: [0]
>>> bridge[1]: lt9611uxc_bridge_funcs
>>>     type: [11] HDMI-A
>>>     OF: 
>>> /soc@0/geniqup@9c0000/i2c@994000/hdmi-bridge@2b:lontium,lt9611uxc
>>>     ops: [7] detect edid hpd
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
>>>   drivers/gpu/drm/drm_debugfs.c | 39 ++++++++++++++++++++++++++++---
>>>   include/drm/drm_bridge.h      |  2 --
>>>   3 files changed, 36 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 39e68e45bb12..cee3188adf3d 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -1347,50 +1347,6 @@ struct drm_bridge *of_drm_find_bridge(struct 
>>> device_node *np)
>>>   EXPORT_SYMBOL(of_drm_find_bridge);
>>>   #endif
>>> -#ifdef CONFIG_DEBUG_FS
>>> -static int drm_bridge_chains_info(struct seq_file *m, void *data)
>>> -{
>>> -    struct drm_debugfs_entry *entry = m->private;
>>> -    struct drm_device *dev = entry->dev;
>>> -    struct drm_printer p = drm_seq_file_printer(m);
>>> -    struct drm_mode_config *config = &dev->mode_config;
>>> -    struct drm_encoder *encoder;
>>> -    unsigned int bridge_idx = 0;
>>> -
>>> -    list_for_each_entry(encoder, &config->encoder_list, head) {
>>> -        struct drm_bridge *bridge;
>>> -
>>> -        drm_printf(&p, "encoder[%u]\n", encoder->base.id);
>>> -
>>> -        drm_for_each_bridge_in_chain(encoder, bridge) {
>>> -            drm_printf(&p, "\tbridge[%u] type: %u, ops: %#x",
>>> -                   bridge_idx, bridge->type, bridge->ops);
>>> -
>>> -#ifdef CONFIG_OF
>>> -            if (bridge->of_node)
>>> -                drm_printf(&p, ", OF: %pOFfc", bridge->of_node);
>>> -#endif
>>> -
>>> -            drm_printf(&p, "\n");
>>> -
>>> -            bridge_idx++;
>>> -        }
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static const struct drm_debugfs_info drm_bridge_debugfs_list[] = {
>>> -    { "bridge_chains", drm_bridge_chains_info, 0 },
>>> -};
>>> -
>>> -void drm_bridge_debugfs_init(struct drm_minor *minor)
>>> -{
>>> -    drm_debugfs_add_files(minor->dev, drm_bridge_debugfs_list,
>>> -                  ARRAY_SIZE(drm_bridge_debugfs_list));
>>> -}
>>> -#endif
>>> -
>>>   MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
>>>   MODULE_DESCRIPTION("DRM bridge infrastructure");
>>>   MODULE_LICENSE("GPL and additional rights");
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>>> b/drivers/gpu/drm/drm_debugfs.c
>>> index cf7f33bdc963..70913067406d 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -273,10 +273,8 @@ int drm_debugfs_init(struct drm_minor *minor, 
>>> int minor_id,
>>>       drm_debugfs_add_files(minor->dev, drm_debugfs_list, 
>>> DRM_DEBUGFS_ENTRIES);
>>> -    if (drm_drv_uses_atomic_modeset(dev)) {
>>> +    if (drm_drv_uses_atomic_modeset(dev))
>>>           drm_atomic_debugfs_init(minor);
>>> -        drm_bridge_debugfs_init(minor);
>>> -    }
>>>       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>           drm_framebuffer_debugfs_init(minor);
>>> @@ -603,6 +601,37 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
>>>       crtc->debugfs_entry = NULL;
>>>   }
>>> +static int bridges_show(struct seq_file *m, void *data)
>>> +{
>>> +    struct drm_encoder *encoder = m->private;
>>> +    struct drm_bridge *bridge;
>>> +    unsigned int idx = 0;
>>> +
>>> +    drm_for_each_bridge_in_chain(encoder, bridge) {
>>> +        seq_printf(m, "bridge[%d]: %ps\n", idx++, bridge->funcs);
>>> +        seq_printf(m, "\ttype: [%d] %s\n",
>>> +               bridge->type,
>>> +               drm_get_connector_type_name(bridge->type));
>>> +#ifdef CONFIG_OF
>>> +        if (bridge->of_node)
>>> +            seq_printf(m, "\tOF: %pOFfc\n", bridge->of_node);
>>> +#endif
>>> +        seq_printf(m, "\tops: [0x%x]", bridge->ops);
>>> +        if (bridge->ops & DRM_BRIDGE_OP_DETECT)
>>> +            seq_puts(m, " detect");
>>> +        if (bridge->ops & DRM_BRIDGE_OP_EDID)
>>> +            seq_puts(m, " edid");
>>> +        if (bridge->ops & DRM_BRIDGE_OP_HPD)
>>> +            seq_puts(m, " hpd");
>>> +        if (bridge->ops & DRM_BRIDGE_OP_MODES)
>>> +            seq_puts(m, " modes");
>>> +        seq_puts(m, "\n");
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(bridges);
>>> +
>>>   void drm_debugfs_encoder_add(struct drm_encoder *encoder)
>>>   {
>>>       struct drm_minor *minor = encoder->dev->primary;
>>> @@ -618,6 +647,10 @@ void drm_debugfs_encoder_add(struct drm_encoder 
>>> *encoder)
>>>       encoder->debugfs_entry = root;
>>> +    /* bridges list */
>>> +    debugfs_create_file("bridges", 0444, root, encoder,
>>> +                &bridges_fops);
>>> +
>>>       if (encoder->funcs->debugfs_init)
>>>           encoder->funcs->debugfs_init(encoder, root);
>>>   }
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index c339fc85fd07..902bc3f99c2a 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -950,6 +950,4 @@ static inline struct drm_bridge 
>>> *drmm_of_get_bridge(struct drm_device *drm,
>>>   }
>>>   #endif
>>> -void drm_bridge_debugfs_init(struct drm_minor *minor);
>>> -
>>>   #endif
>>
>> It would be nice to have a review from Tomi since he pushed the bridge 
>> chains debugfs.
>>
>> Apart that it looks fine:
>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> This change does more than move the code to per-encoder debugfs file: it 
> changes the formatting, adding textual representations for the flags, 
> and drops the use of drm_printer.
> 
> I'd prefer to have such changes separately, but as it's a small patch I 
> guess it's fine-ish. But they should at least be mentioned in the patch 
> description.

Fair enough, I'll add this to the commit message. Thank you!

> 
> With that addressed:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>   Tomi
> 

-- 
With best wishes
Dmitry