[PATCH v2 1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()

[PATCH v2 1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()

[Dmitry Baryshkov]

This callback has been unused since the driver being added. Drop it now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 7 -------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 3 ---
 2 files changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1dc5dbe58572..c771383446f2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2115,7 +2115,6 @@ DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
 static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-	int i;
 
 	char name[DPU_NAME_SIZE];
 
@@ -2134,12 +2133,6 @@ static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 	debugfs_create_file("status", 0600,
 		dpu_enc->debugfs_root, dpu_enc, &_dpu_encoder_status_fops);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++)
-		if (dpu_enc->phys_encs[i]->ops.late_register)
-			dpu_enc->phys_encs[i]->ops.late_register(
-					dpu_enc->phys_encs[i],
-					dpu_enc->debugfs_root);
-
 	return 0;
 }
 #else
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 1d434b22180d..9e29079a6fc4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -63,7 +63,6 @@ struct dpu_encoder_phys;
 /**
  * struct dpu_encoder_phys_ops - Interface the physical encoders provide to
  *	the containing virtual encoder.
- * @late_register:		DRM Call. Add Userspace interfaces, debugfs.
  * @prepare_commit:		MSM Atomic Call, start of atomic commit sequence
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
@@ -93,8 +92,6 @@ struct dpu_encoder_phys;
  */
 
 struct dpu_encoder_phys_ops {
-	int (*late_register)(struct dpu_encoder_phys *encoder,
-			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
 	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
-- 
2.39.2

[PATCH v2 2/3] drm/msm/dpu: drop (mostly) unused DPU_NAME_SIZE define

[Dmitry Baryshkov]

This define is used only in one place, in dpu_encoder debugfs code.
Inline the value and drop the define completely.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     | 2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 2 --
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c771383446f2..af34932729db 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2116,14 +2116,14 @@ static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
 
-	char name[DPU_NAME_SIZE];
+	char name[12];
 
 	if (!drm_enc->dev) {
 		DPU_ERROR("invalid encoder or kms\n");
 		return -EINVAL;
 	}
 
-	snprintf(name, DPU_NAME_SIZE, "encoder%u", drm_enc->base.id);
+	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,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 66209e2448d2..c4f82180ad10 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -63,8 +63,6 @@
 #define ktime_compare_safe(A, B) \
 	ktime_compare(ktime_sub((A), (B)), ktime_set(0, 0))
 
-#define DPU_NAME_SIZE  12
-
 struct dpu_kms {
 	struct msm_kms base;
 	struct drm_device *dev;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 14b5cfe30611..ac75ba13aa01 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -42,8 +42,6 @@
 #define SHARP_SMOOTH_THR_DEFAULT	8
 #define SHARP_NOISE_THR_DEFAULT	2
 
-#define DPU_NAME_SIZE  12
-
 #define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
 #define DPU_ZPOS_MAX 255
 
-- 
2.39.2

Re: [PATCH v2 2/3] drm/msm/dpu: drop (mostly) unused DPU_NAME_SIZE define

[Marijn Suijten]

On 2023-05-21 22:22:29, Dmitry Baryshkov wrote:
> This define is used only in one place, in dpu_encoder debugfs code.
> Inline the value and drop the define completely.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Even if this was a constant, always a fan of sizeof(name) versus
assuming that name has some length constant/define!

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     | 2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 2 --
>  3 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index c771383446f2..af34932729db 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2116,14 +2116,14 @@ static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>  {
>  	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>  
> -	char name[DPU_NAME_SIZE];
> +	char name[12];
>  
>  	if (!drm_enc->dev) {
>  		DPU_ERROR("invalid encoder or kms\n");
>  		return -EINVAL;
>  	}
>  
> -	snprintf(name, DPU_NAME_SIZE, "encoder%u", drm_enc->base.id);
> +	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,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 66209e2448d2..c4f82180ad10 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -63,8 +63,6 @@
>  #define ktime_compare_safe(A, B) \
>  	ktime_compare(ktime_sub((A), (B)), ktime_set(0, 0))
>  
> -#define DPU_NAME_SIZE  12
> -
>  struct dpu_kms {
>  	struct msm_kms base;
>  	struct drm_device *dev;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 14b5cfe30611..ac75ba13aa01 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -42,8 +42,6 @@
>  #define SHARP_SMOOTH_THR_DEFAULT	8
>  #define SHARP_NOISE_THR_DEFAULT	2
>  
> -#define DPU_NAME_SIZE  12
> -
>  #define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
>  #define DPU_ZPOS_MAX 255
>  
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/3] drm/msm/dpu: drop (mostly) unused DPU_NAME_SIZE define

[Abhinav Kumar]

On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
> This define is used only in one place, in dpu_encoder debugfs code.
> Inline the value and drop the define completely.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

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

[PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Dmitry Baryshkov]

Use drm_debugfs_add_file() for encoder's status file. This changes the
name of the status file from encoder%d/status to just encoder%d.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index af34932729db..0ac68f44ec74 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -14,6 +14,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_debugfs.h>
 #include <drm/drm_file.h>
 #include <drm/drm_probe_helper.h>
 
@@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
  * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
  *				all CTL paths
  * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
- * @debugfs_root:		Debug file system root file node
  * @enc_lock:			Lock around physical encoder
  *				create/destroy/enable/disable
  * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
@@ -186,7 +186,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);
@@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
+	struct dpu_encoder_virt *dpu_enc = entry->file.data;
 	int i;
 
 	mutex_lock(&dpu_enc->enc_lock);
@@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
-
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-
-	char name[12];
+	char *name;
 
 	if (!drm_enc->dev) {
 		DPU_ERROR("invalid encoder or kms\n");
-		return -EINVAL;
+		return;
 	}
 
-	snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
+	name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
+	drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
 }
 #else
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
 {
-	return 0;
 }
 #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);
+	_dpu_encoder_init_debugfs(encoder);
 
-	debugfs_remove_recursive(dpu_enc->debugfs_root);
+	return 0;
 }
 
 static int dpu_encoder_virt_add_phys_encs(
@@ -2380,7 +2363,6 @@ 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,
 };
 
 int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
-- 
2.39.2

Re: [PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Marijn Suijten]

On 2023-05-21 22:22:30, Dmitry Baryshkov wrote:
> Use drm_debugfs_add_file() for encoder's status file. This changes the
> name of the status file from encoder%d/status to just encoder%d.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Also nice that this is a managed variant so that we don't have to clean
up the debugfs entries manually.

There's also an unused @debugfs_root: doc in dpu_core_perf.h, though
that has nothing to do with this patch.

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
>  1 file changed, 11 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index af34932729db..0ac68f44ec74 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -14,6 +14,7 @@
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
>   * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
>   *				all CTL paths
>   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> - * @debugfs_root:		Debug file system root file node
>   * @enc_lock:			Lock around physical encoder
>   *				create/destroy/enable/disable
>   * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
> @@ -186,7 +186,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);
> @@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
> +	struct dpu_encoder_virt *dpu_enc = entry->file.data;
>  	int i;
>  
>  	mutex_lock(&dpu_enc->enc_lock);
> @@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>  	return 0;
>  }
>  
> -DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
> -
> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>  {
>  	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> -
> -	char name[12];
> +	char *name;
>  
>  	if (!drm_enc->dev) {
>  		DPU_ERROR("invalid encoder or kms\n");
> -		return -EINVAL;
> +		return;
>  	}
>  
> -	snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
> +	name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
> +	drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
>  }
>  #else
> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>  {
> -	return 0;
>  }
>  #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);
> +	_dpu_encoder_init_debugfs(encoder);
>  
> -	debugfs_remove_recursive(dpu_enc->debugfs_root);
> +	return 0;
>  }
>  
>  static int dpu_encoder_virt_add_phys_encs(
> @@ -2380,7 +2363,6 @@ 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,
>  };
>  
>  int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> -- 
> 2.39.2
> 

Re: [PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Abhinav Kumar]

On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
> Use drm_debugfs_add_file() for encoder's status file. This changes the
> name of the status file from encoder%d/status to just encoder%d.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

This patch depends on 
https://patchwork.freedesktop.org/patch/538294/?series=118079&rev=1 right?

What is wrong with having a per encoder directory and reading from 
there? It gives room for expanding this to dump more encoder specific 
information.

At the moment it looks light because we have only status but better to 
have a directory per encoder right?

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
>   1 file changed, 11 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index af34932729db..0ac68f44ec74 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -14,6 +14,7 @@
>   
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_crtc.h>
> +#include <drm/drm_debugfs.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_probe_helper.h>
>   
> @@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
>    * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
>    *				all CTL paths
>    * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> - * @debugfs_root:		Debug file system root file node
>    * @enc_lock:			Lock around physical encoder
>    *				create/destroy/enable/disable
>    * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
> @@ -186,7 +186,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);
> @@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
> +	struct dpu_encoder_virt *dpu_enc = entry->file.data;
>   	int i;
>   
>   	mutex_lock(&dpu_enc->enc_lock);
> @@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>   	return 0;
>   }
>   
> -DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
> -
> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>   {
>   	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> -
> -	char name[12];
> +	char *name;
>   
>   	if (!drm_enc->dev) {
>   		DPU_ERROR("invalid encoder or kms\n");
> -		return -EINVAL;
> +		return;
>   	}
>   
> -	snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
> +	name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
> +	drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
>   }
>   #else
> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>   {
> -	return 0;
>   }
>   #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);
> +	_dpu_encoder_init_debugfs(encoder);
>   
> -	debugfs_remove_recursive(dpu_enc->debugfs_root);
> +	return 0;
>   }
>   
>   static int dpu_encoder_virt_add_phys_encs(
> @@ -2380,7 +2363,6 @@ 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,
>   };
>   
>   int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,

Re: [PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Dmitry Baryshkov]

On Wed, 24 May 2023 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
> > Use drm_debugfs_add_file() for encoder's status file. This changes the
> > name of the status file from encoder%d/status to just encoder%d.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> This patch depends on
> https://patchwork.freedesktop.org/patch/538294/?series=118079&rev=1 right?

No, there is no dependency. I have sent that patch as we discussed it
earlier. But this one is a reimplementation of the previous idea.

>
> What is wrong with having a per encoder directory and reading from
> there? It gives room for expanding this to dump more encoder specific
> information.
>
> At the moment it looks light because we have only status but better to
> have a directory per encoder right?

I started writing that I can not imagine additional per-encoder data,
but then I found the generic enough piece: bridge chain enumeration.
I'll give it additional thought and maybe I'll refactor this patch
further.

>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
> >   1 file changed, 11 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index af34932729db..0ac68f44ec74 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -14,6 +14,7 @@
> >
> >   #include <drm/drm_atomic.h>
> >   #include <drm/drm_crtc.h>
> > +#include <drm/drm_debugfs.h>
> >   #include <drm/drm_file.h>
> >   #include <drm/drm_probe_helper.h>
> >
> > @@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
> >    * @crtc_kickoff_cb:                Callback into CRTC that will flush & start
> >    *                          all CTL paths
> >    * @crtc_kickoff_cb_data:   Opaque user data given to crtc_kickoff_cb
> > - * @debugfs_root:            Debug file system root file node
> >    * @enc_lock:                       Lock around physical encoder
> >    *                          create/destroy/enable/disable
> >    * @frame_busy_mask:                Bitmask tracking which phys_enc we are still
> > @@ -186,7 +186,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);
> > @@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
> > +     struct dpu_encoder_virt *dpu_enc = entry->file.data;
> >       int i;
> >
> >       mutex_lock(&dpu_enc->enc_lock);
> > @@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> >       return 0;
> >   }
> >
> > -DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
> > -
> > -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> > +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >   {
> >       struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > -
> > -     char name[12];
> > +     char *name;
> >
> >       if (!drm_enc->dev) {
> >               DPU_ERROR("invalid encoder or kms\n");
> > -             return -EINVAL;
> > +             return;
> >       }
> >
> > -     snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
> > +     name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
> > +     drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
> >   }
> >   #else
> > -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> > +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >   {
> > -     return 0;
> >   }
> >   #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);
> > +     _dpu_encoder_init_debugfs(encoder);
> >
> > -     debugfs_remove_recursive(dpu_enc->debugfs_root);
> > +     return 0;
> >   }
> >
> >   static int dpu_encoder_virt_add_phys_encs(
> > @@ -2380,7 +2363,6 @@ 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,
> >   };
> >
> >   int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,



-- 
With best wishes
Dmitry

Re: [PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Abhinav Kumar]

On 5/23/2023 4:53 PM, Dmitry Baryshkov wrote:
> On Wed, 24 May 2023 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
>>> Use drm_debugfs_add_file() for encoder's status file. This changes the
>>> name of the status file from encoder%d/status to just encoder%d.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> This patch depends on
>> https://patchwork.freedesktop.org/patch/538294/?series=118079&rev=1 right?
> 
> No, there is no dependency. I have sent that patch as we discussed it
> earlier. But this one is a reimplementation of the previous idea.
> 

In this patch you are also removing the early_unregister callback.

.early_unregister = dpu_encoder_early_unregister

Which we discussed was needed to balance the corner case we discussed. 
The DRM core patch fixes the corner case by calling debugfs_cleanup() 
even when drm_modeset_register_all() fails.

So isnt there a dependency?

>>
>> What is wrong with having a per encoder directory and reading from
>> there? It gives room for expanding this to dump more encoder specific
>> information.
>>
>> At the moment it looks light because we have only status but better to
>> have a directory per encoder right?
> 
> I started writing that I can not imagine additional per-encoder data,
> but then I found the generic enough piece: bridge chain enumeration.
> I'll give it additional thought and maybe I'll refactor this patch
> further.
> 

Ack,
>>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
>>>    1 file changed, 11 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index af34932729db..0ac68f44ec74 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -14,6 +14,7 @@
>>>
>>>    #include <drm/drm_atomic.h>
>>>    #include <drm/drm_crtc.h>
>>> +#include <drm/drm_debugfs.h>
>>>    #include <drm/drm_file.h>
>>>    #include <drm/drm_probe_helper.h>
>>>
>>> @@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
>>>     * @crtc_kickoff_cb:                Callback into CRTC that will flush & start
>>>     *                          all CTL paths
>>>     * @crtc_kickoff_cb_data:   Opaque user data given to crtc_kickoff_cb
>>> - * @debugfs_root:            Debug file system root file node
>>>     * @enc_lock:                       Lock around physical encoder
>>>     *                          create/destroy/enable/disable
>>>     * @frame_busy_mask:                Bitmask tracking which phys_enc we are still
>>> @@ -186,7 +186,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);
>>> @@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
>>> +     struct dpu_encoder_virt *dpu_enc = entry->file.data;
>>>        int i;
>>>
>>>        mutex_lock(&dpu_enc->enc_lock);
>>> @@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>>>        return 0;
>>>    }
>>>
>>> -DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
>>> -
>>> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>>> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>>>    {
>>>        struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>>> -
>>> -     char name[12];
>>> +     char *name;
>>>
>>>        if (!drm_enc->dev) {
>>>                DPU_ERROR("invalid encoder or kms\n");
>>> -             return -EINVAL;
>>> +             return;
>>>        }
>>>
>>> -     snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
>>> +     name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
>>> +     drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
>>>    }
>>>    #else
>>> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>>> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>>>    {
>>> -     return 0;
>>>    }
>>>    #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);
>>> +     _dpu_encoder_init_debugfs(encoder);
>>>
>>> -     debugfs_remove_recursive(dpu_enc->debugfs_root);
>>> +     return 0;
>>>    }
>>>
>>>    static int dpu_encoder_virt_add_phys_encs(
>>> @@ -2380,7 +2363,6 @@ 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,
>>>    };
>>>
>>>    int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> 
> 
> 

Re: [PATCH v2 3/3] drm/msm/dpu: switch dpu_encoder to use drm_debugfs_add_file()

[Dmitry Baryshkov]

On Wed, 24 May 2023 at 03:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/23/2023 4:53 PM, Dmitry Baryshkov wrote:
> > On Wed, 24 May 2023 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
> >>> Use drm_debugfs_add_file() for encoder's status file. This changes the
> >>> name of the status file from encoder%d/status to just encoder%d.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>
> >> This patch depends on
> >> https://patchwork.freedesktop.org/patch/538294/?series=118079&rev=1 right?
> >
> > No, there is no dependency. I have sent that patch as we discussed it
> > earlier. But this one is a reimplementation of the previous idea.
> >
>
> In this patch you are also removing the early_unregister callback.
>
> .early_unregister = dpu_encoder_early_unregister
>
> Which we discussed was needed to balance the corner case we discussed.
> The DRM core patch fixes the corner case by calling debugfs_cleanup()
> even when drm_modeset_register_all() fails.
>
> So isnt there a dependency?

No. There is no remove counterpart for drm_debugfs_add_file(). DRM
subsystem handles everything internally.

>
> >>
> >> What is wrong with having a per encoder directory and reading from
> >> there? It gives room for expanding this to dump more encoder specific
> >> information.
> >>
> >> At the moment it looks light because we have only status but better to
> >> have a directory per encoder right?
> >
> > I started writing that I can not imagine additional per-encoder data,
> > but then I found the generic enough piece: bridge chain enumeration.
> > I'll give it additional thought and maybe I'll refactor this patch
> > further.
> >
>
> Ack,
> >>
> >>> ---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
> >>>    1 file changed, 11 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> index af34932729db..0ac68f44ec74 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> @@ -14,6 +14,7 @@
> >>>
> >>>    #include <drm/drm_atomic.h>
> >>>    #include <drm/drm_crtc.h>
> >>> +#include <drm/drm_debugfs.h>
> >>>    #include <drm/drm_file.h>
> >>>    #include <drm/drm_probe_helper.h>
> >>>
> >>> @@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
> >>>     * @crtc_kickoff_cb:                Callback into CRTC that will flush & start
> >>>     *                          all CTL paths
> >>>     * @crtc_kickoff_cb_data:   Opaque user data given to crtc_kickoff_cb
> >>> - * @debugfs_root:            Debug file system root file node
> >>>     * @enc_lock:                       Lock around physical encoder
> >>>     *                          create/destroy/enable/disable
> >>>     * @frame_busy_mask:                Bitmask tracking which phys_enc we are still
> >>> @@ -186,7 +186,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);
> >>> @@ -2091,7 +2090,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_debugfs_entry *entry = s->private;
> >>> +     struct dpu_encoder_virt *dpu_enc = entry->file.data;
> >>>        int i;
> >>>
> >>>        mutex_lock(&dpu_enc->enc_lock);
> >>> @@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> >>>        return 0;
> >>>    }
> >>>
> >>> -DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
> >>> -
> >>> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >>> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >>>    {
> >>>        struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> >>> -
> >>> -     char name[12];
> >>> +     char *name;
> >>>
> >>>        if (!drm_enc->dev) {
> >>>                DPU_ERROR("invalid encoder or kms\n");
> >>> -             return -EINVAL;
> >>> +             return;
> >>>        }
> >>>
> >>> -     snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
> >>> +     name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "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;
> >>> +     drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
> >>>    }
> >>>    #else
> >>> -static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >>> +static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
> >>>    {
> >>> -     return 0;
> >>>    }
> >>>    #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);
> >>> +     _dpu_encoder_init_debugfs(encoder);
> >>>
> >>> -     debugfs_remove_recursive(dpu_enc->debugfs_root);
> >>> +     return 0;
> >>>    }
> >>>
> >>>    static int dpu_encoder_virt_add_phys_encs(
> >>> @@ -2380,7 +2363,6 @@ 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,
> >>>    };
> >>>
> >>>    int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()

[Marijn Suijten]

On 2023-05-21 22:22:28, Dmitry Baryshkov wrote:
> This callback has been unused since the driver being added. Drop it now.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 7 -------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 3 ---
>  2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1dc5dbe58572..c771383446f2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2115,7 +2115,6 @@ DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
>  static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>  {
>  	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> -	int i;
>  
>  	char name[DPU_NAME_SIZE];
>  
> @@ -2134,12 +2133,6 @@ static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
>  	debugfs_create_file("status", 0600,
>  		dpu_enc->debugfs_root, dpu_enc, &_dpu_encoder_status_fops);
>  
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++)
> -		if (dpu_enc->phys_encs[i]->ops.late_register)
> -			dpu_enc->phys_encs[i]->ops.late_register(
> -					dpu_enc->phys_encs[i],
> -					dpu_enc->debugfs_root);
> -
>  	return 0;
>  }
>  #else
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 1d434b22180d..9e29079a6fc4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -63,7 +63,6 @@ struct dpu_encoder_phys;
>  /**
>   * struct dpu_encoder_phys_ops - Interface the physical encoders provide to
>   *	the containing virtual encoder.
> - * @late_register:		DRM Call. Add Userspace interfaces, debugfs.
>   * @prepare_commit:		MSM Atomic Call, start of atomic commit sequence
>   * @is_master:			Whether this phys_enc is the current master
>   *				encoder. Can be switched at enable time. Based
> @@ -93,8 +92,6 @@ struct dpu_encoder_phys;
>   */
>  
>  struct dpu_encoder_phys_ops {
> -	int (*late_register)(struct dpu_encoder_phys *encoder,
> -			struct dentry *debugfs_root);
>  	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>  	bool (*is_master)(struct dpu_encoder_phys *encoder);
>  	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
> -- 
> 2.39.2
> 

Re: [PATCH v2 1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()

[Abhinav Kumar]

On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
> This callback has been unused since the driver being added. Drop it now.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

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

Re: [PATCH v2 1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()

[Dmitry Baryshkov]

On Sun, 21 May 2023 22:22:28 +0300, Dmitry Baryshkov wrote:
> This callback has been unused since the driver being added. Drop it now.
> 
> 

Applied, thanks!

[1/3] drm/msm/dpu: drop dpu_encoder_phys_ops::late_register()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/95666ca7431c
[2/3] drm/msm/dpu: drop (mostly) unused DPU_NAME_SIZE define
      https://gitlab.freedesktop.org/lumag/msm/-/commit/a659098d78d6

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