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

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

[Dmitry Baryshkov]

Resending, patch 1 needs review from DRM core maintainers, but it got no
attention since October.

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.

Changes since v1:
- Brought back drm_printer usage to bridges_show (Tomi Valkeinen)
- Updated the drm/bridge commit message to reflect format changes (Tomi
  Valkeinen)

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               | 65 ++++++++++++++++++++-
 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, 96 insertions(+), 89 deletions(-)

-- 
2.39.2

[PATCH RESEND v2 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.

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
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 f291fb4b359f..00406b4f3235 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -589,4 +589,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 b12c463bc460..7df17e4b0513 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -194,6 +194,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 void drm_debugfs_dev_fini(struct drm_device *dev)
 {
@@ -231,6 +233,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 RESEND v2 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.

While we are at it, also slightly improve the formatting of the bridge
data: split a single line entry into multiple lines, include the symbol
name of the bridge funcs and add the textual representation of the
bridge ops.

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

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge.c  | 44 -----------------------------------
 drivers/gpu/drm/drm_debugfs.c | 40 ++++++++++++++++++++++++++++---
 include/drm/drm_bridge.h      |  2 --
 3 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 30d66bee0ec6..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_device *dev)
-{
-	drm_debugfs_add_files(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 00406b4f3235..02e7481758c0 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -314,10 +314,8 @@ void drm_debugfs_dev_register(struct drm_device *dev)
 		drm_framebuffer_debugfs_init(dev);
 		drm_client_debugfs_init(dev);
 	}
-	if (drm_drv_uses_atomic_modeset(dev)) {
+	if (drm_drv_uses_atomic_modeset(dev))
 		drm_atomic_debugfs_init(dev);
-		drm_bridge_debugfs_init(dev);
-	}
 }
 
 int drm_debugfs_register(struct drm_minor *minor, int minor_id,
@@ -589,6 +587,38 @@ 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_printer p = drm_seq_file_printer(m);
+	struct drm_bridge *bridge;
+	unsigned int idx = 0;
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		drm_printf(&p, "bridge[%d]: %ps\n", idx++, bridge->funcs);
+		drm_printf(&p, "\ttype: [%d] %s\n",
+			   bridge->type,
+			   drm_get_connector_type_name(bridge->type));
+#ifdef CONFIG_OF
+		if (bridge->of_node)
+			drm_printf(&p, "\tOF: %pOFfc\n", bridge->of_node);
+#endif
+		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");
+		drm_puts(&p, "\n");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(bridges);
+
 void drm_debugfs_encoder_add(struct drm_encoder *encoder)
 {
 	struct drm_minor *minor = encoder->dev->primary;
@@ -604,6 +634,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 9ef461aa9b9e..e39da5807ba7 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_device *dev);
-
 #endif
-- 
2.39.2

[PATCH RESEND v2 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 1cf7ff6caff4..498983e62f7e 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);
@@ -2108,7 +2107,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);
@@ -2130,48 +2130,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,
@@ -2355,8 +2323,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 RESEND v2 1/3] drm/encoder: register per-encoder debugfs dir

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]

Hi,

On Sun, Dec 03, 2023 at 02:53:13PM +0300, 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.
> 
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> 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 f291fb4b359f..00406b4f3235 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -589,4 +589,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 b12c463bc460..7df17e4b0513 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -194,6 +194,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 void drm_debugfs_dev_fini(struct drm_device *dev)
>  {
> @@ -231,6 +233,13 @@ static inline void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>  {
>  }
>  
> +static inline void drm_debugfs_encoder_add(struct drm_encoder *encoder)
> +{
> +}

<- You need to insert a new line here.

Once fixed,
Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: (subset) [PATCH RESEND v2 0/3] drm: introduce per-encoder debugfs directory

[Dmitry Baryshkov]

On Sun, 03 Dec 2023 14:53:12 +0300, Dmitry Baryshkov wrote:
> Resending, patch 1 needs review from DRM core maintainers, but it got no
> attention since October.
> 
> 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.
> 
> [...]

Applied to drm-misc-next, thanks!

[1/3] drm/encoder: register per-encoder debugfs dir
      commit: caf525ed45b4960b450cbd4e811d9b247bc2586c
[2/3] drm/bridge: migrate bridge_chains to per-encoder file
      commit: d0b3c318e04cc6c4e2a3c30ee0f6f619aa8d0db5

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

[Abhinav Kumar]

On 12/3/2023 3:53 AM, Dmitry Baryshkov wrote:
> 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(-)
> 

For this change,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Looking more closely at other drivers, most of them (atleast what I 
checked) were doing the same functionality in drm_encoder's 
late_register / early_unregister as DPU.

This can be a wider cleanup across the tree if needed or we can stop here.

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

[Dmitry Baryshkov]

On Tue, 5 Dec 2023 at 01:36, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/3/2023 3:53 AM, Dmitry Baryshkov wrote:
> > 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(-)
> >
>
> For this change,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> Looking more closely at other drivers, most of them (atleast what I
> checked) were doing the same functionality in drm_encoder's
> late_register / early_unregister as DPU.
>
> This can be a wider cleanup across the tree if needed or we can stop here.

Yes, that's why I thought that this is a good idea. I think I'll let
other driver maintainers rework their drivers.

-- 
With best wishes
Dmitry

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4192 bytes --]

On Sun, Dec 03, 2023 at 02:53:13PM +0300, 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.

Today's -next fails to boot an imx_v6_v7_defconfig on at least the UDOO
dual and quad platforms, based on i.MX6DL and i.MX6Q respectively.
multi_v7_defconfig looks fine on the same boards, it's just the i.MX
specific config that's failing.  Nothing else in my CI appears impacted.
We get a NULL pointer defererence while bringing up the display
subsystem:

[    1.392715] imx-drm display-subsystem: bound imx-ipuv3-crtc.7 (ops 0xc0f9a490)
[    1.400013] imx-drm display-subsystem: bound 120000.hdmi (ops 0xc0f9af80)
[    1.407193] 8<--- cut here ---
[    1.410256] Unable to handle kernel NULL pointer dereference at virtual address 00000010 when read

...

[    1.891882]  drm_debugfs_encoder_add from drm_encoder_register_all+0x20/0x60
[    1.898954]  drm_encoder_register_all from drm_modeset_register_all+0x34/0x70
[    1.906116]  drm_modeset_register_all from drm_dev_register+0x140/0x288
[    1.912765]  drm_dev_register from imx_drm_bind+0xd0/0x128
[    1.918284]  imx_drm_bind from try_to_bring_up_aggregate_device+0x164/0x1c4
[    1.925275]  try_to_bring_up_aggregate_device from __component_add+0x90/0x13c

Full log at:

   https://lava.sirena.org.uk/scheduler/job/308781

A bisect identfied this patch (in -next as caf525ed45b4960b4) as being
the commit that introduced the issue, bisect log below.  I've not done
any other investigation but the commit does seem plausibly related to
the backtrace in the oops.

git bisect start
# good: [cc1b39317a57120651840e79b535594ee09f5768] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
git bisect good cc1b39317a57120651840e79b535594ee09f5768
# bad: [0f5f12ac05f36f117e793656c3f560625e927f1b] Add linux-next specific files for 20231205
git bisect bad 0f5f12ac05f36f117e793656c3f560625e927f1b
# good: [8390406ed4d9d360bc404a5a3e9b82f335a8d417] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 8390406ed4d9d360bc404a5a3e9b82f335a8d417
# bad: [b948f47bf8abdaf87f74c553b589c50567090aa2] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
git bisect bad b948f47bf8abdaf87f74c553b589c50567090aa2
# bad: [dce94061f0d02f5ab355390a6e63d3dbea938b72] drm/v3d: Fix missing error code in v3d_submit_cpu_ioctl()
git bisect bad dce94061f0d02f5ab355390a6e63d3dbea938b72
# good: [0d3abd456be45369235dd75793ce26f07900044c] drm/imagination: vm: fix drm_gpuvm reference count
git bisect good 0d3abd456be45369235dd75793ce26f07900044c
# good: [f52ffea0745943bb6af674f30f4243b3721b7cd6] drm/i915/iosf: Drop unused APIs
git bisect good f52ffea0745943bb6af674f30f4243b3721b7cd6
# good: [b101d08451de6eaebd1a840e4885ce7ce73656ad] drm/nouveau: Removes unnecessary args check in nouveau_uvmm_sm_prepare
git bisect good b101d08451de6eaebd1a840e4885ce7ce73656ad
# good: [63ee44540205d993854f143a5ab1d7d9e63ffcf1] dma-buf/sync_file: Add SET_DEADLINE ioctl
git bisect good 63ee44540205d993854f143a5ab1d7d9e63ffcf1
# good: [e4256751df4a0a3860f181588ee730dd19cb0c30] drm/display/dp: Add the remaining Square PHY patterns DPCD register definitions
git bisect good e4256751df4a0a3860f181588ee730dd19cb0c30
# good: [2bcca96abfbf89d26fc10fc92e40532bb2ae8891] soc: qcom: pmic-glink: switch to DRM_AUX_HPD_BRIDGE
git bisect good 2bcca96abfbf89d26fc10fc92e40532bb2ae8891
# bad: [b881ba8faa5c7689eb1cb487ad891c46dbbed0e8] Revert "drm/atomic: Move framebuffer checks to helper"
git bisect bad b881ba8faa5c7689eb1cb487ad891c46dbbed0e8
# bad: [caf525ed45b4960b450cbd4e811d9b247bc2586c] drm/encoder: register per-encoder debugfs dir
git bisect bad caf525ed45b4960b450cbd4e811d9b247bc2586c
# good: [7d9f1b72b29698e3030c2b163522cf4aa91b47e9] usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE
git bisect good 7d9f1b72b29698e3030c2b163522cf4aa91b47e9
# first bad commit: [caf525ed45b4960b450cbd4e811d9b247bc2586c] drm/encoder: register per-encoder debugfs dir

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

Hi Mark,

On Tue, Dec 05, 2023 at 04:13:41PM +0000, Mark Brown wrote:
> On Sun, Dec 03, 2023 at 02:53:13PM +0300, 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.
> 
> Today's -next fails to boot an imx_v6_v7_defconfig on at least the UDOO
> dual and quad platforms, based on i.MX6DL and i.MX6Q respectively.
> multi_v7_defconfig looks fine on the same boards, it's just the i.MX
> specific config that's failing.  Nothing else in my CI appears impacted.
> We get a NULL pointer defererence while bringing up the display
> subsystem:
> 
> [    1.392715] imx-drm display-subsystem: bound imx-ipuv3-crtc.7 (ops 0xc0f9a490)
> [    1.400013] imx-drm display-subsystem: bound 120000.hdmi (ops 0xc0f9af80)
> [    1.407193] 8<--- cut here ---
> [    1.410256] Unable to handle kernel NULL pointer dereference at virtual address 00000010 when read
> 
> ...
> 
> [    1.891882]  drm_debugfs_encoder_add from drm_encoder_register_all+0x20/0x60
> [    1.898954]  drm_encoder_register_all from drm_modeset_register_all+0x34/0x70
> [    1.906116]  drm_modeset_register_all from drm_dev_register+0x140/0x288
> [    1.912765]  drm_dev_register from imx_drm_bind+0xd0/0x128
> [    1.918284]  imx_drm_bind from try_to_bring_up_aggregate_device+0x164/0x1c4
> [    1.925275]  try_to_bring_up_aggregate_device from __component_add+0x90/0x13c
> 
> Full log at:
> 
>    https://lava.sirena.org.uk/scheduler/job/308781
> 
> A bisect identfied this patch (in -next as caf525ed45b4960b4) as being
> the commit that introduced the issue, bisect log below.  I've not done
> any other investigation but the commit does seem plausibly related to
> the backtrace in the oops.

I think it's the same issue than the one fixed by:
https://lore.kernel.org/all/20231205130631.3456986-1-m.szyprowski@samsung.com/

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dmitry Baryshkov]

On Sun, 03 Dec 2023 14:53:12 +0300, Dmitry Baryshkov wrote:
> Resending, patch 1 needs review from DRM core maintainers, but it got no
> attention since October.
> 
> 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.
> 
> [...]

Applied, thanks!

[3/3] drm/msm/dpu: move encoder status to standard encoder debugfs dir
      https://gitlab.freedesktop.org/lumag/msm/-/commit/62d35629da80

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>