[PATCH v1 0/8] drm/msm/dpu: cleanup debugfs code

[PATCH v1 0/8] drm/msm/dpu: cleanup debugfs code

[Dmitry Baryshkov]

This patchset provides a partial rework/cleanup/fixup of debugfs code in
DPU driver. It started as a single patch removing (and then just moving)
SSPP debugfs code from the plane (as planes are going to be less
connected with SSPP blocks soon). However the more I touched this code,
the more patches were generated as more and more issues arrive on the
surface.

The following changes since commit fee32807633395e666f0951d6b7b6546e9b76c3d:

  mailmap: add and update email addresses (2021-11-29 16:19:58 -0800)

are available in the Git repository at:

  https://git.linaro.org/people/dmitry.baryshkov/kernel.git dpu-cleanup-debugfs

for you to fetch changes up to 7f3598ee9ea525920cd6fa65b498604a9ff8b36a:

  drm/msm/dpu: move SSPP debugfs support from plane to SSPP code (2021-12-02 01:03:49 +0300)

----------------------------------------------------------------
Dmitry Baryshkov (8):
      drm/msm/dpu: move disable_danger out of plane subdir
      drm/msm/dpu: fix safe status debugfs file
      drm/msm/dpu: make danger_status/safe_status readable
      drm/msm/dpu: drop plane's default_scaling debugfs file
      drm/msm/dpu: stop manually removing debugfs files for the DPU plane
      drm/msm/dpu: stop manually removing debugfs files for the DPU CRTC
      drm/msm/dpu: simplify DPU's regset32 code
      drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  15 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |   3 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c |  67 +++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |   4 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 109 +++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  38 +-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 172 ++--------------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   6 +
 8 files changed, 189 insertions(+), 225 deletions(-)

[PATCH v1 1/8] drm/msm/dpu: move disable_danger out of plane subdir

[Dmitry Baryshkov]

The disable_danger debugfs file is not related to a single plane.
Instead it is used by all registered planes. Move it from plane subtree
to the global subtree next to danger_status and safe_status files,
so that the new file supplements them.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6c457c419412..259d438bc6e8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -101,6 +101,73 @@ static int dpu_debugfs_safe_stats_show(struct seq_file *s, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(dpu_debugfs_safe_stats);
 
+static ssize_t _dpu_plane_danger_read(struct file *file,
+			char __user *buff, size_t count, loff_t *ppos)
+{
+	struct dpu_kms *kms = file->private_data;
+	int len;
+	char buf[40];
+
+	len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl);
+
+	return simple_read_from_buffer(buff, count, ppos, buf, len);
+}
+
+static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable)
+{
+	struct drm_plane *plane;
+
+	drm_for_each_plane(plane, kms->dev) {
+		if (plane->fb && plane->state) {
+			dpu_plane_danger_signal_ctrl(plane, enable);
+			DPU_DEBUG("plane:%d img:%dx%d ",
+				plane->base.id, plane->fb->width,
+				plane->fb->height);
+			DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n",
+				plane->state->src_x >> 16,
+				plane->state->src_y >> 16,
+				plane->state->src_w >> 16,
+				plane->state->src_h >> 16,
+				plane->state->crtc_x, plane->state->crtc_y,
+				plane->state->crtc_w, plane->state->crtc_h);
+		} else {
+			DPU_DEBUG("Inactive plane:%d\n", plane->base.id);
+		}
+	}
+}
+
+static ssize_t _dpu_plane_danger_write(struct file *file,
+		    const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	struct dpu_kms *kms = file->private_data;
+	int disable_panic;
+	int ret;
+
+	ret = kstrtouint_from_user(user_buf, count, 0, &disable_panic);
+	if (ret)
+		return ret;
+
+	if (disable_panic) {
+		/* Disable panic signal for all active pipes */
+		DPU_DEBUG("Disabling danger:\n");
+		_dpu_plane_set_danger_state(kms, false);
+		kms->has_danger_ctrl = false;
+	} else {
+		/* Enable panic signal for all active pipes */
+		DPU_DEBUG("Enabling danger:\n");
+		kms->has_danger_ctrl = true;
+		_dpu_plane_set_danger_state(kms, true);
+	}
+
+	return count;
+}
+
+static const struct file_operations dpu_plane_danger_enable = {
+	.open = simple_open,
+	.read = _dpu_plane_danger_read,
+	.write = _dpu_plane_danger_write,
+};
+
 static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
 		struct dentry *parent)
 {
@@ -110,6 +177,9 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
 			dpu_kms, &dpu_debugfs_danger_stats_fops);
 	debugfs_create_file("safe_status", 0600, entry,
 			dpu_kms, &dpu_debugfs_safe_stats_fops);
+	debugfs_create_file("disable_danger", 0600, entry,
+			dpu_kms, &dpu_plane_danger_enable);
+
 }
 
 static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ca190d92f0d5..6ea4db061c9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1350,7 +1350,7 @@ static void dpu_plane_reset(struct drm_plane *plane)
 }
 
 #ifdef CONFIG_DEBUG_FS
-static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
+void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 {
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
@@ -1363,73 +1363,6 @@ static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
-static ssize_t _dpu_plane_danger_read(struct file *file,
-			char __user *buff, size_t count, loff_t *ppos)
-{
-	struct dpu_kms *kms = file->private_data;
-	int len;
-	char buf[40];
-
-	len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl);
-
-	return simple_read_from_buffer(buff, count, ppos, buf, len);
-}
-
-static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable)
-{
-	struct drm_plane *plane;
-
-	drm_for_each_plane(plane, kms->dev) {
-		if (plane->fb && plane->state) {
-			dpu_plane_danger_signal_ctrl(plane, enable);
-			DPU_DEBUG("plane:%d img:%dx%d ",
-				plane->base.id, plane->fb->width,
-				plane->fb->height);
-			DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n",
-				plane->state->src_x >> 16,
-				plane->state->src_y >> 16,
-				plane->state->src_w >> 16,
-				plane->state->src_h >> 16,
-				plane->state->crtc_x, plane->state->crtc_y,
-				plane->state->crtc_w, plane->state->crtc_h);
-		} else {
-			DPU_DEBUG("Inactive plane:%d\n", plane->base.id);
-		}
-	}
-}
-
-static ssize_t _dpu_plane_danger_write(struct file *file,
-		    const char __user *user_buf, size_t count, loff_t *ppos)
-{
-	struct dpu_kms *kms = file->private_data;
-	int disable_panic;
-	int ret;
-
-	ret = kstrtouint_from_user(user_buf, count, 0, &disable_panic);
-	if (ret)
-		return ret;
-
-	if (disable_panic) {
-		/* Disable panic signal for all active pipes */
-		DPU_DEBUG("Disabling danger:\n");
-		_dpu_plane_set_danger_state(kms, false);
-		kms->has_danger_ctrl = false;
-	} else {
-		/* Enable panic signal for all active pipes */
-		DPU_DEBUG("Enabling danger:\n");
-		kms->has_danger_ctrl = true;
-		_dpu_plane_set_danger_state(kms, true);
-	}
-
-	return count;
-}
-
-static const struct file_operations dpu_plane_danger_enable = {
-	.open = simple_open,
-	.read = _dpu_plane_danger_read,
-	.write = _dpu_plane_danger_write,
-};
-
 static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 {
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
@@ -1498,11 +1431,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 			pdpu->debugfs_root,
 			(u32 *) &sblk->danger_vblank);
 
-	debugfs_create_file("disable_danger",
-			0600,
-			pdpu->debugfs_root,
-			kms, &dpu_plane_danger_enable);
-
 	return 0;
 }
 #else
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
index 52792526e904..7667b1f81bd4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -132,4 +132,10 @@ void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state);
 int dpu_plane_color_fill(struct drm_plane *plane,
 		uint32_t color, uint32_t alpha);
 
+#ifdef CONFIG_DEBUG_FS
+void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable);
+#else
+static inline void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) {}
+#endif
+
 #endif /* _DPU_PLANE_H_ */
-- 
2.33.0

Re: [Freedreno] [PATCH v1 1/8] drm/msm/dpu: move disable_danger out of plane subdir

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> The disable_danger debugfs file is not related to a single plane.
> Instead it is used by all registered planes. Move it from plane subtree
> to the global subtree next to danger_status and safe_status files,
> so that the new file supplements them.
> 
the danger_safe itself is a per-plane feature as each SSPP can be 
controlled individually.

In todays code, yes we do it for all the active planes together.
Since this is the same behavior

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

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 70 +++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +----------------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |  6 ++
>   3 files changed, 77 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 6c457c419412..259d438bc6e8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -101,6 +101,73 @@ static int dpu_debugfs_safe_stats_show(struct seq_file *s, void *v)
>   }
>   DEFINE_SHOW_ATTRIBUTE(dpu_debugfs_safe_stats);
>   
> +static ssize_t _dpu_plane_danger_read(struct file *file,
> +			char __user *buff, size_t count, loff_t *ppos)
> +{
> +	struct dpu_kms *kms = file->private_data;
> +	int len;
> +	char buf[40];
> +
> +	len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl);
> +
> +	return simple_read_from_buffer(buff, count, ppos, buf, len);
> +}
> +
> +static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable)
> +{
> +	struct drm_plane *plane;
> +
> +	drm_for_each_plane(plane, kms->dev) {
> +		if (plane->fb && plane->state) {
> +			dpu_plane_danger_signal_ctrl(plane, enable);
> +			DPU_DEBUG("plane:%d img:%dx%d ",
> +				plane->base.id, plane->fb->width,
> +				plane->fb->height);
> +			DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n",
> +				plane->state->src_x >> 16,
> +				plane->state->src_y >> 16,
> +				plane->state->src_w >> 16,
> +				plane->state->src_h >> 16,
> +				plane->state->crtc_x, plane->state->crtc_y,
> +				plane->state->crtc_w, plane->state->crtc_h);
> +		} else {
> +			DPU_DEBUG("Inactive plane:%d\n", plane->base.id);
> +		}
> +	}
> +}
> +
> +static ssize_t _dpu_plane_danger_write(struct file *file,
> +		    const char __user *user_buf, size_t count, loff_t *ppos)
> +{
> +	struct dpu_kms *kms = file->private_data;
> +	int disable_panic;
> +	int ret;
> +
> +	ret = kstrtouint_from_user(user_buf, count, 0, &disable_panic);
> +	if (ret)
> +		return ret;
> +
> +	if (disable_panic) {
> +		/* Disable panic signal for all active pipes */
> +		DPU_DEBUG("Disabling danger:\n");
> +		_dpu_plane_set_danger_state(kms, false);
> +		kms->has_danger_ctrl = false;
> +	} else {
> +		/* Enable panic signal for all active pipes */
> +		DPU_DEBUG("Enabling danger:\n");
> +		kms->has_danger_ctrl = true;
> +		_dpu_plane_set_danger_state(kms, true);
> +	}
> +
> +	return count;
> +}
> +
> +static const struct file_operations dpu_plane_danger_enable = {
> +	.open = simple_open,
> +	.read = _dpu_plane_danger_read,
> +	.write = _dpu_plane_danger_write,
> +};
> +
>   static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
>   		struct dentry *parent)
>   {
> @@ -110,6 +177,9 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
>   			dpu_kms, &dpu_debugfs_danger_stats_fops);
>   	debugfs_create_file("safe_status", 0600, entry,
>   			dpu_kms, &dpu_debugfs_safe_stats_fops);
> +	debugfs_create_file("disable_danger", 0600, entry,
> +			dpu_kms, &dpu_plane_danger_enable);
> +
>   }
>   
>   static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index ca190d92f0d5..6ea4db061c9f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1350,7 +1350,7 @@ static void dpu_plane_reset(struct drm_plane *plane)
>   }
>   
>   #ifdef CONFIG_DEBUG_FS
> -static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> +void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>   {
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
>   	struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
> @@ -1363,73 +1363,6 @@ static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
>   
> -static ssize_t _dpu_plane_danger_read(struct file *file,
> -			char __user *buff, size_t count, loff_t *ppos)
> -{
> -	struct dpu_kms *kms = file->private_data;
> -	int len;
> -	char buf[40];
> -
> -	len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl);
> -
> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> -}
> -
> -static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable)
> -{
> -	struct drm_plane *plane;
> -
> -	drm_for_each_plane(plane, kms->dev) {
> -		if (plane->fb && plane->state) {
> -			dpu_plane_danger_signal_ctrl(plane, enable);
> -			DPU_DEBUG("plane:%d img:%dx%d ",
> -				plane->base.id, plane->fb->width,
> -				plane->fb->height);
> -			DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n",
> -				plane->state->src_x >> 16,
> -				plane->state->src_y >> 16,
> -				plane->state->src_w >> 16,
> -				plane->state->src_h >> 16,
> -				plane->state->crtc_x, plane->state->crtc_y,
> -				plane->state->crtc_w, plane->state->crtc_h);
> -		} else {
> -			DPU_DEBUG("Inactive plane:%d\n", plane->base.id);
> -		}
> -	}
> -}
> -
> -static ssize_t _dpu_plane_danger_write(struct file *file,
> -		    const char __user *user_buf, size_t count, loff_t *ppos)
> -{
> -	struct dpu_kms *kms = file->private_data;
> -	int disable_panic;
> -	int ret;
> -
> -	ret = kstrtouint_from_user(user_buf, count, 0, &disable_panic);
> -	if (ret)
> -		return ret;
> -
> -	if (disable_panic) {
> -		/* Disable panic signal for all active pipes */
> -		DPU_DEBUG("Disabling danger:\n");
> -		_dpu_plane_set_danger_state(kms, false);
> -		kms->has_danger_ctrl = false;
> -	} else {
> -		/* Enable panic signal for all active pipes */
> -		DPU_DEBUG("Enabling danger:\n");
> -		kms->has_danger_ctrl = true;
> -		_dpu_plane_set_danger_state(kms, true);
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations dpu_plane_danger_enable = {
> -	.open = simple_open,
> -	.read = _dpu_plane_danger_read,
> -	.write = _dpu_plane_danger_write,
> -};
> -
>   static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   {
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
> @@ -1498,11 +1431,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   			pdpu->debugfs_root,
>   			(u32 *) &sblk->danger_vblank);
>   
> -	debugfs_create_file("disable_danger",
> -			0600,
> -			pdpu->debugfs_root,
> -			kms, &dpu_plane_danger_enable);
> -
>   	return 0;
>   }
>   #else
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> index 52792526e904..7667b1f81bd4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> @@ -132,4 +132,10 @@ void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state);
>   int dpu_plane_color_fill(struct drm_plane *plane,
>   		uint32_t color, uint32_t alpha);
>   
> +#ifdef CONFIG_DEBUG_FS
> +void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable);
> +#else
> +static inline void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) {}
> +#endif
> +
>   #endif /* _DPU_PLANE_H_ */
> 

[PATCH v1 2/8] drm/msm/dpu: fix safe status debugfs file

[Dmitry Baryshkov]

Make safe_status debugfs fs file actually return safe status rather than
danger status data.

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 259d438bc6e8..e7f0cded2c6b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -73,8 +73,8 @@ static int _dpu_danger_signal_status(struct seq_file *s,
 					&status);
 	} else {
 		seq_puts(s, "\nSafe signal status:\n");
-		if (kms->hw_mdp->ops.get_danger_status)
-			kms->hw_mdp->ops.get_danger_status(kms->hw_mdp,
+		if (kms->hw_mdp->ops.get_safe_status)
+			kms->hw_mdp->ops.get_safe_status(kms->hw_mdp,
 					&status);
 	}
 	pm_runtime_put_sync(&kms->pdev->dev);
-- 
2.33.0

Re: [Freedreno] [PATCH v1 2/8] drm/msm/dpu: fix safe status debugfs file

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> Make safe_status debugfs fs file actually return safe status rather than
> danger status data.
> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 259d438bc6e8..e7f0cded2c6b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -73,8 +73,8 @@ static int _dpu_danger_signal_status(struct seq_file *s,
>   					&status);
>   	} else {
>   		seq_puts(s, "\nSafe signal status:\n");
> -		if (kms->hw_mdp->ops.get_danger_status)
> -			kms->hw_mdp->ops.get_danger_status(kms->hw_mdp,
> +		if (kms->hw_mdp->ops.get_safe_status)
> +			kms->hw_mdp->ops.get_safe_status(kms->hw_mdp,
>   					&status);
>   	}
>   	pm_runtime_put_sync(&kms->pdev->dev);
> 

[PATCH v1 3/8] drm/msm/dpu: make danger_status/safe_status readable

[Dmitry Baryshkov]

Change \t to \n in the print format to stop putting all SSPP status in a
single line. Splitting it to one SSPP per line is much more readable.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e7f0cded2c6b..4c04982c71b2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -82,7 +82,7 @@ static int _dpu_danger_signal_status(struct seq_file *s,
 	seq_printf(s, "MDP     :  0x%x\n", status.mdp);
 
 	for (i = SSPP_VIG0; i < SSPP_MAX; i++)
-		seq_printf(s, "SSPP%d   :  0x%x  \t", i - SSPP_VIG0,
+		seq_printf(s, "SSPP%d   :  0x%x  \n", i - SSPP_VIG0,
 				status.sspp[i]);
 	seq_puts(s, "\n");
 
-- 
2.33.0

Re: [PATCH v1 3/8] drm/msm/dpu: make danger_status/safe_status readable

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> Change \t to \n in the print format to stop putting all SSPP status in a
> single line. Splitting it to one SSPP per line is much more readable.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e7f0cded2c6b..4c04982c71b2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -82,7 +82,7 @@ static int _dpu_danger_signal_status(struct seq_file *s,
>   	seq_printf(s, "MDP     :  0x%x\n", status.mdp);
>   
>   	for (i = SSPP_VIG0; i < SSPP_MAX; i++)
> -		seq_printf(s, "SSPP%d   :  0x%x  \t", i - SSPP_VIG0,
> +		seq_printf(s, "SSPP%d   :  0x%x  \n", i - SSPP_VIG0,
>   				status.sspp[i]);
>   	seq_puts(s, "\n");
>   
> 

[PATCH v1 4/8] drm/msm/dpu: drop plane's default_scaling debugfs file

[Dmitry Baryshkov]

Proper support for the 'default_scaling' debugfs file was removed during
DPU driver pre-merge cleanup. Remove leftover file.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 6ea4db061c9f..f80ee3ba9a8a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -114,7 +114,6 @@ struct dpu_plane {
 	struct dpu_debugfs_regset32 debugfs_src;
 	struct dpu_debugfs_regset32 debugfs_scaler;
 	struct dpu_debugfs_regset32 debugfs_csc;
-	bool debugfs_default_scale;
 };
 
 static const uint64_t supported_format_modifiers[] = {
@@ -1398,10 +1397,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 		dpu_debugfs_create_regset32("scaler_blk", 0400,
 				pdpu->debugfs_root,
 				&pdpu->debugfs_scaler);
-		debugfs_create_bool("default_scaling",
-				0600,
-				pdpu->debugfs_root,
-				&pdpu->debugfs_default_scale);
 	}
 
 	if (cfg->features & BIT(DPU_SSPP_CSC) ||
-- 
2.33.0

Re: [Freedreno] [PATCH v1 4/8] drm/msm/dpu: drop plane's default_scaling debugfs file

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> Proper support for the 'default_scaling' debugfs file was removed during
> DPU driver pre-merge cleanup. Remove leftover file.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 6ea4db061c9f..f80ee3ba9a8a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -114,7 +114,6 @@ struct dpu_plane {
>   	struct dpu_debugfs_regset32 debugfs_src;
>   	struct dpu_debugfs_regset32 debugfs_scaler;
>   	struct dpu_debugfs_regset32 debugfs_csc;
> -	bool debugfs_default_scale;
>   };
>   
>   static const uint64_t supported_format_modifiers[] = {
> @@ -1398,10 +1397,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   		dpu_debugfs_create_regset32("scaler_blk", 0400,
>   				pdpu->debugfs_root,
>   				&pdpu->debugfs_scaler);
> -		debugfs_create_bool("default_scaling",
> -				0600,
> -				pdpu->debugfs_root,
> -				&pdpu->debugfs_default_scale);
>   	}
>   
>   	if (cfg->features & BIT(DPU_SSPP_CSC) ||
> 

[PATCH v1 5/8] drm/msm/dpu: stop manually removing debugfs files for the DPU plane

[Dmitry Baryshkov]

DRM code handles removing all debugfs recursively. Drop plane-specific
code to perform that.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index f80ee3ba9a8a..d3176f58708e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -110,7 +110,6 @@ struct dpu_plane {
 	struct dpu_mdss_cfg *catalog;
 
 	/* debugfs related stuff */
-	struct dentry *debugfs_root;
 	struct dpu_debugfs_regset32 debugfs_src;
 	struct dpu_debugfs_regset32 debugfs_scaler;
 	struct dpu_debugfs_regset32 debugfs_csc;
@@ -1368,15 +1367,16 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 	struct dpu_kms *kms = _dpu_plane_get_kms(plane);
 	const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
 	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+	struct dentry *debugfs_root;
 
 	/* create overall sub-directory for the pipe */
-	pdpu->debugfs_root =
+	debugfs_root =
 		debugfs_create_dir(plane->name,
 				plane->dev->primary->debugfs_root);
 
 	/* don't error check these */
 	debugfs_create_xul("features", 0600,
-			pdpu->debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
+			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
 
 	/* add register dump support */
 	dpu_debugfs_setup_regset32(&pdpu->debugfs_src,
@@ -1384,7 +1384,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 			sblk->src_blk.len,
 			kms);
 	dpu_debugfs_create_regset32("src_blk", 0400,
-			pdpu->debugfs_root, &pdpu->debugfs_src);
+			debugfs_root, &pdpu->debugfs_src);
 
 	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
 			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
@@ -1395,7 +1395,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 				sblk->scaler_blk.len,
 				kms);
 		dpu_debugfs_create_regset32("scaler_blk", 0400,
-				pdpu->debugfs_root,
+				debugfs_root,
 				&pdpu->debugfs_scaler);
 	}
 
@@ -1406,24 +1406,24 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 				sblk->csc_blk.len,
 				kms);
 		dpu_debugfs_create_regset32("csc_blk", 0400,
-				pdpu->debugfs_root, &pdpu->debugfs_csc);
+				debugfs_root, &pdpu->debugfs_csc);
 	}
 
 	debugfs_create_u32("xin_id",
 			0400,
-			pdpu->debugfs_root,
+			debugfs_root,
 			(u32 *) &cfg->xin_id);
 	debugfs_create_u32("clk_ctrl",
 			0400,
-			pdpu->debugfs_root,
+			debugfs_root,
 			(u32 *) &cfg->clk_ctrl);
 	debugfs_create_x32("creq_vblank",
 			0600,
-			pdpu->debugfs_root,
+			debugfs_root,
 			(u32 *) &sblk->creq_vblank);
 	debugfs_create_x32("danger_vblank",
 			0600,
-			pdpu->debugfs_root,
+			debugfs_root,
 			(u32 *) &sblk->danger_vblank);
 
 	return 0;
@@ -1440,13 +1440,6 @@ static int dpu_plane_late_register(struct drm_plane *plane)
 	return _dpu_plane_init_debugfs(plane);
 }
 
-static void dpu_plane_early_unregister(struct drm_plane *plane)
-{
-	struct dpu_plane *pdpu = to_dpu_plane(plane);
-
-	debugfs_remove_recursive(pdpu->debugfs_root);
-}
-
 static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
 		uint32_t format, uint64_t modifier)
 {
@@ -1472,7 +1465,6 @@ static const struct drm_plane_funcs dpu_plane_funcs = {
 		.atomic_duplicate_state = dpu_plane_duplicate_state,
 		.atomic_destroy_state = dpu_plane_destroy_state,
 		.late_register = dpu_plane_late_register,
-		.early_unregister = dpu_plane_early_unregister,
 		.format_mod_supported = dpu_plane_format_mod_supported,
 };
 
-- 
2.33.0

Re: [PATCH v1 5/8] drm/msm/dpu: stop manually removing debugfs files for the DPU plane

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> DRM code handles removing all debugfs recursively. Drop plane-specific
> code to perform that.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 28 ++++++++---------------
>   1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index f80ee3ba9a8a..d3176f58708e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -110,7 +110,6 @@ struct dpu_plane {
>   	struct dpu_mdss_cfg *catalog;
>   
>   	/* debugfs related stuff */
> -	struct dentry *debugfs_root;
>   	struct dpu_debugfs_regset32 debugfs_src;
>   	struct dpu_debugfs_regset32 debugfs_scaler;
>   	struct dpu_debugfs_regset32 debugfs_csc;
> @@ -1368,15 +1367,16 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   	struct dpu_kms *kms = _dpu_plane_get_kms(plane);
>   	const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
>   	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
> +	struct dentry *debugfs_root;
>   
>   	/* create overall sub-directory for the pipe */
> -	pdpu->debugfs_root =
> +	debugfs_root =
>   		debugfs_create_dir(plane->name,
>   				plane->dev->primary->debugfs_root);
>   
>   	/* don't error check these */
>   	debugfs_create_xul("features", 0600,
> -			pdpu->debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
> +			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
>   
>   	/* add register dump support */
>   	dpu_debugfs_setup_regset32(&pdpu->debugfs_src,
> @@ -1384,7 +1384,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   			sblk->src_blk.len,
>   			kms);
>   	dpu_debugfs_create_regset32("src_blk", 0400,
> -			pdpu->debugfs_root, &pdpu->debugfs_src);
> +			debugfs_root, &pdpu->debugfs_src);
>   
>   	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>   			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
> @@ -1395,7 +1395,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   				sblk->scaler_blk.len,
>   				kms);
>   		dpu_debugfs_create_regset32("scaler_blk", 0400,
> -				pdpu->debugfs_root,
> +				debugfs_root,
>   				&pdpu->debugfs_scaler);
>   	}
>   
> @@ -1406,24 +1406,24 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   				sblk->csc_blk.len,
>   				kms);
>   		dpu_debugfs_create_regset32("csc_blk", 0400,
> -				pdpu->debugfs_root, &pdpu->debugfs_csc);
> +				debugfs_root, &pdpu->debugfs_csc);
>   	}
>   
>   	debugfs_create_u32("xin_id",
>   			0400,
> -			pdpu->debugfs_root,
> +			debugfs_root,
>   			(u32 *) &cfg->xin_id);
>   	debugfs_create_u32("clk_ctrl",
>   			0400,
> -			pdpu->debugfs_root,
> +			debugfs_root,
>   			(u32 *) &cfg->clk_ctrl);
>   	debugfs_create_x32("creq_vblank",
>   			0600,
> -			pdpu->debugfs_root,
> +			debugfs_root,
>   			(u32 *) &sblk->creq_vblank);
>   	debugfs_create_x32("danger_vblank",
>   			0600,
> -			pdpu->debugfs_root,
> +			debugfs_root,
>   			(u32 *) &sblk->danger_vblank);
>   
>   	return 0;
> @@ -1440,13 +1440,6 @@ static int dpu_plane_late_register(struct drm_plane *plane)
>   	return _dpu_plane_init_debugfs(plane);
>   }
>   
> -static void dpu_plane_early_unregister(struct drm_plane *plane)
> -{
> -	struct dpu_plane *pdpu = to_dpu_plane(plane);
> -
> -	debugfs_remove_recursive(pdpu->debugfs_root);
> -}
> -
>   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
>   		uint32_t format, uint64_t modifier)
>   {
> @@ -1472,7 +1465,6 @@ static const struct drm_plane_funcs dpu_plane_funcs = {
>   		.atomic_duplicate_state = dpu_plane_duplicate_state,
>   		.atomic_destroy_state = dpu_plane_destroy_state,
>   		.late_register = dpu_plane_late_register,
> -		.early_unregister = dpu_plane_early_unregister,
>   		.format_mod_supported = dpu_plane_format_mod_supported,
>   };
>   
> 

[PATCH v1 6/8] drm/msm/dpu: stop manually removing debugfs files for the DPU CRTC

[Dmitry Baryshkov]

DRM code handles removing all debugfs recursively. Drop CRTC-specific
code to perform that.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d290809d59bd..9899f7424131 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1424,15 +1424,16 @@ DEFINE_SHOW_ATTRIBUTE(dpu_crtc_debugfs_state);
 static int _dpu_crtc_init_debugfs(struct drm_crtc *crtc)
 {
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+	struct dentry *debugfs_root;
 
-	dpu_crtc->debugfs_root = debugfs_create_dir(dpu_crtc->name,
+	debugfs_root = debugfs_create_dir(dpu_crtc->name,
 			crtc->dev->primary->debugfs_root);
 
 	debugfs_create_file("status", 0400,
-			dpu_crtc->debugfs_root,
+			debugfs_root,
 			dpu_crtc, &_dpu_debugfs_status_fops);
 	debugfs_create_file("state", 0600,
-			dpu_crtc->debugfs_root,
+			debugfs_root,
 			&dpu_crtc->base,
 			&dpu_crtc_debugfs_state_fops);
 
@@ -1450,13 +1451,6 @@ static int dpu_crtc_late_register(struct drm_crtc *crtc)
 	return _dpu_crtc_init_debugfs(crtc);
 }
 
-static void dpu_crtc_early_unregister(struct drm_crtc *crtc)
-{
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-
-	debugfs_remove_recursive(dpu_crtc->debugfs_root);
-}
-
 static const struct drm_crtc_funcs dpu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = dpu_crtc_destroy,
@@ -1465,7 +1459,6 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
 	.atomic_duplicate_state = dpu_crtc_duplicate_state,
 	.atomic_destroy_state = dpu_crtc_destroy_state,
 	.late_register = dpu_crtc_late_register,
-	.early_unregister = dpu_crtc_early_unregister,
 	.verify_crc_source = dpu_crtc_verify_crc_source,
 	.set_crc_source = dpu_crtc_set_crc_source,
 	.enable_vblank  = msm_crtc_enable_vblank,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 4328e133d71c..b8785c394fcc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -129,7 +129,6 @@ struct dpu_crtc_frame_event {
  * @drm_requested_vblank : Whether vblanks have been enabled in the encoder
  * @property_info : Opaque structure for generic property support
  * @property_defaults : Array of default values for generic property support
- * @debugfs_root  : Parent of debugfs node
  * @vblank_cb_count : count of vblank callback since last reset
  * @play_count    : frame count between crtc enable and disable
  * @vblank_cb_time  : ktime at vblank count reset
@@ -160,8 +159,6 @@ struct dpu_crtc {
 	struct drm_pending_vblank_event *event;
 	u32 vsync_count;
 
-	struct dentry *debugfs_root;
-
 	u32 vblank_cb_count;
 	u64 play_count;
 	ktime_t vblank_cb_time;
-- 
2.33.0

Re: [Freedreno] [PATCH v1 6/8] drm/msm/dpu: stop manually removing debugfs files for the DPU CRTC

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> DRM code handles removing all debugfs recursively. Drop CRTC-specific
> code to perform that.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++-----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h |  3 ---
>   2 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d290809d59bd..9899f7424131 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1424,15 +1424,16 @@ DEFINE_SHOW_ATTRIBUTE(dpu_crtc_debugfs_state);
>   static int _dpu_crtc_init_debugfs(struct drm_crtc *crtc)
>   {
>   	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dentry *debugfs_root;
>   
> -	dpu_crtc->debugfs_root = debugfs_create_dir(dpu_crtc->name,
> +	debugfs_root = debugfs_create_dir(dpu_crtc->name,
>   			crtc->dev->primary->debugfs_root);
>   
>   	debugfs_create_file("status", 0400,
> -			dpu_crtc->debugfs_root,
> +			debugfs_root,
>   			dpu_crtc, &_dpu_debugfs_status_fops);
>   	debugfs_create_file("state", 0600,
> -			dpu_crtc->debugfs_root,
> +			debugfs_root,
>   			&dpu_crtc->base,
>   			&dpu_crtc_debugfs_state_fops);
>   
> @@ -1450,13 +1451,6 @@ static int dpu_crtc_late_register(struct drm_crtc *crtc)
>   	return _dpu_crtc_init_debugfs(crtc);
>   }
>   
> -static void dpu_crtc_early_unregister(struct drm_crtc *crtc)
> -{
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -
> -	debugfs_remove_recursive(dpu_crtc->debugfs_root);
> -}
> -
>   static const struct drm_crtc_funcs dpu_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.destroy = dpu_crtc_destroy,
> @@ -1465,7 +1459,6 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
>   	.atomic_duplicate_state = dpu_crtc_duplicate_state,
>   	.atomic_destroy_state = dpu_crtc_destroy_state,
>   	.late_register = dpu_crtc_late_register,
> -	.early_unregister = dpu_crtc_early_unregister,
>   	.verify_crc_source = dpu_crtc_verify_crc_source,
>   	.set_crc_source = dpu_crtc_set_crc_source,
>   	.enable_vblank  = msm_crtc_enable_vblank,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 4328e133d71c..b8785c394fcc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -129,7 +129,6 @@ struct dpu_crtc_frame_event {
>    * @drm_requested_vblank : Whether vblanks have been enabled in the encoder
>    * @property_info : Opaque structure for generic property support
>    * @property_defaults : Array of default values for generic property support
> - * @debugfs_root  : Parent of debugfs node
>    * @vblank_cb_count : count of vblank callback since last reset
>    * @play_count    : frame count between crtc enable and disable
>    * @vblank_cb_time  : ktime at vblank count reset
> @@ -160,8 +159,6 @@ struct dpu_crtc {
>   	struct drm_pending_vblank_event *event;
>   	u32 vsync_count;
>   
> -	struct dentry *debugfs_root;
> -
>   	u32 vblank_cb_count;
>   	u64 play_count;
>   	ktime_t vblank_cb_time;
> 

[PATCH v1 7/8] drm/msm/dpu: simplify DPU's regset32 code

[Dmitry Baryshkov]

Squash dpu_debugfs_setup_regset32() into dpu_debugfs_create_regset32().
it makes little sense to have separate function to just setup the
structure.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 4c04982c71b2..7e7a619769a8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -182,6 +182,15 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
 
 }
 
+/*
+ * Companion structure for dpu_debugfs_create_regset32.
+ */
+struct dpu_debugfs_regset32 {
+	uint32_t offset;
+	uint32_t blk_len;
+	struct dpu_kms *dpu_kms;
+};
+
 static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
 {
 	struct dpu_debugfs_regset32 *regset = s->private;
@@ -229,24 +238,23 @@ static const struct file_operations dpu_fops_regset32 = {
 	.release =	single_release,
 };
 
-void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
+void dpu_debugfs_create_regset32(const char *name, umode_t mode,
+		void *parent,
 		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms)
 {
-	if (regset) {
-		regset->offset = offset;
-		regset->blk_len = length;
-		regset->dpu_kms = dpu_kms;
-	}
-}
+	struct dpu_debugfs_regset32 *regset;
 
-void dpu_debugfs_create_regset32(const char *name, umode_t mode,
-		void *parent, struct dpu_debugfs_regset32 *regset)
-{
-	if (!name || !regset || !regset->dpu_kms || !regset->blk_len)
+	if (WARN_ON(!name || !dpu_kms || !length))
+		return;
+
+	regset = devm_kzalloc(&dpu_kms->pdev->dev, sizeof(*regset), GFP_KERNEL);
+	if (!regset)
 		return;
 
 	/* make sure offset is a multiple of 4 */
-	regset->offset = round_down(regset->offset, 4);
+	regset->offset = round_down(offset, 4);
+	regset->blk_len = length;
+	regset->dpu_kms = dpu_kms;
 
 	debugfs_create_file(name, mode, parent, regset, &dpu_fops_regset32);
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 775bcbda860f..b53cdeb1b5c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -160,33 +160,9 @@ struct dpu_global_state
  *
  * Documentation/filesystems/debugfs.rst
  *
- * @dpu_debugfs_setup_regset32: Initialize data for dpu_debugfs_create_regset32
  * @dpu_debugfs_create_regset32: Create 32-bit register dump file
- * @dpu_debugfs_get_root: Get root dentry for DPU_KMS's debugfs node
  */
 
-/**
- * Companion structure for dpu_debugfs_create_regset32. Do not initialize the
- * members of this structure explicitly; use dpu_debugfs_setup_regset32 instead.
- */
-struct dpu_debugfs_regset32 {
-	uint32_t offset;
-	uint32_t blk_len;
-	struct dpu_kms *dpu_kms;
-};
-
-/**
- * dpu_debugfs_setup_regset32 - Initialize register block definition for debugfs
- * This function is meant to initialize dpu_debugfs_regset32 structures for use
- * with dpu_debugfs_create_regset32.
- * @regset: opaque register definition structure
- * @offset: sub-block offset
- * @length: sub-block length, in bytes
- * @dpu_kms: pointer to dpu kms structure
- */
-void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
-		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms);
-
 /**
  * dpu_debugfs_create_regset32 - Create register read back file for debugfs
  *
@@ -195,20 +171,16 @@ void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
  * names/offsets do not need to be provided. The 'read' function simply outputs
  * sequential register values over a specified range.
  *
- * Similar to the related debugfs_create_regset32 API, the structure pointed to
- * by regset needs to persist for the lifetime of the created file. The calling
- * code is responsible for initialization/management of this structure.
- *
- * The structure pointed to by regset is meant to be opaque. Please use
- * dpu_debugfs_setup_regset32 to initialize it.
- *
  * @name:   File name within debugfs
  * @mode:   File mode within debugfs
  * @parent: Parent directory entry within debugfs, can be NULL
- * @regset: Pointer to persistent register block definition
+ * @offset: sub-block offset
+ * @length: sub-block length, in bytes
+ * @dpu_kms: pointer to dpu kms structure
  */
 void dpu_debugfs_create_regset32(const char *name, umode_t mode,
-		void *parent, struct dpu_debugfs_regset32 *regset);
+		void *parent,
+		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms);
 
 /**
  * dpu_debugfs_get_root - Return root directory entry for KMS's debugfs
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index d3176f58708e..ef66af696a40 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -108,11 +108,6 @@ struct dpu_plane {
 	bool is_virtual;
 	struct list_head mplane_list;
 	struct dpu_mdss_cfg *catalog;
-
-	/* debugfs related stuff */
-	struct dpu_debugfs_regset32 debugfs_src;
-	struct dpu_debugfs_regset32 debugfs_scaler;
-	struct dpu_debugfs_regset32 debugfs_csc;
 };
 
 static const uint64_t supported_format_modifiers[] = {
@@ -1379,35 +1374,29 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
 			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
 
 	/* add register dump support */
-	dpu_debugfs_setup_regset32(&pdpu->debugfs_src,
+	dpu_debugfs_create_regset32("src_blk", 0400,
+			debugfs_root,
 			sblk->src_blk.base + cfg->base,
 			sblk->src_blk.len,
 			kms);
-	dpu_debugfs_create_regset32("src_blk", 0400,
-			debugfs_root, &pdpu->debugfs_src);
 
 	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
 			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
 			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
-			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4)) {
-		dpu_debugfs_setup_regset32(&pdpu->debugfs_scaler,
+			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+		dpu_debugfs_create_regset32("scaler_blk", 0400,
+				debugfs_root,
 				sblk->scaler_blk.base + cfg->base,
 				sblk->scaler_blk.len,
 				kms);
-		dpu_debugfs_create_regset32("scaler_blk", 0400,
-				debugfs_root,
-				&pdpu->debugfs_scaler);
-	}
 
 	if (cfg->features & BIT(DPU_SSPP_CSC) ||
-			cfg->features & BIT(DPU_SSPP_CSC_10BIT)) {
-		dpu_debugfs_setup_regset32(&pdpu->debugfs_csc,
+			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
+		dpu_debugfs_create_regset32("csc_blk", 0400,
+				debugfs_root,
 				sblk->csc_blk.base + cfg->base,
 				sblk->csc_blk.len,
 				kms);
-		dpu_debugfs_create_regset32("csc_blk", 0400,
-				debugfs_root, &pdpu->debugfs_csc);
-	}
 
 	debugfs_create_u32("xin_id",
 			0400,
-- 
2.33.0

Re: [Freedreno] [PATCH v1 7/8] drm/msm/dpu: simplify DPU's regset32 code

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> Squash dpu_debugfs_setup_regset32() into dpu_debugfs_create_regset32().
> it makes little sense to have separate function to just setup the
> structure.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 32 ++++++++++++-------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   | 38 +++--------------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 +++++-----------
>   3 files changed, 33 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 4c04982c71b2..7e7a619769a8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -182,6 +182,15 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
>   
>   }
>   
> +/*
> + * Companion structure for dpu_debugfs_create_regset32.
> + */
> +struct dpu_debugfs_regset32 {
> +	uint32_t offset;
> +	uint32_t blk_len;
> +	struct dpu_kms *dpu_kms;
> +};
> +
>   static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
>   {
>   	struct dpu_debugfs_regset32 *regset = s->private;
> @@ -229,24 +238,23 @@ static const struct file_operations dpu_fops_regset32 = {
>   	.release =	single_release,
>   };
>   
> -void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
> +void dpu_debugfs_create_regset32(const char *name, umode_t mode,
> +		void *parent,
>   		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms)
>   {
> -	if (regset) {
> -		regset->offset = offset;
> -		regset->blk_len = length;
> -		regset->dpu_kms = dpu_kms;
> -	}
> -}
> +	struct dpu_debugfs_regset32 *regset;
>   
> -void dpu_debugfs_create_regset32(const char *name, umode_t mode,
> -		void *parent, struct dpu_debugfs_regset32 *regset)
> -{
> -	if (!name || !regset || !regset->dpu_kms || !regset->blk_len)
> +	if (WARN_ON(!name || !dpu_kms || !length))
> +		return;
> +
> +	regset = devm_kzalloc(&dpu_kms->pdev->dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
>   		return;
>   
>   	/* make sure offset is a multiple of 4 */
> -	regset->offset = round_down(regset->offset, 4);
> +	regset->offset = round_down(offset, 4);
> +	regset->blk_len = length;
> +	regset->dpu_kms = dpu_kms;
>   
>   	debugfs_create_file(name, mode, parent, regset, &dpu_fops_regset32);
>   }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 775bcbda860f..b53cdeb1b5c4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -160,33 +160,9 @@ struct dpu_global_state
>    *
>    * Documentation/filesystems/debugfs.rst
>    *
> - * @dpu_debugfs_setup_regset32: Initialize data for dpu_debugfs_create_regset32
>    * @dpu_debugfs_create_regset32: Create 32-bit register dump file
> - * @dpu_debugfs_get_root: Get root dentry for DPU_KMS's debugfs node
>    */
>   
> -/**
> - * Companion structure for dpu_debugfs_create_regset32. Do not initialize the
> - * members of this structure explicitly; use dpu_debugfs_setup_regset32 instead.
> - */
> -struct dpu_debugfs_regset32 {
> -	uint32_t offset;
> -	uint32_t blk_len;
> -	struct dpu_kms *dpu_kms;
> -};
> -
> -/**
> - * dpu_debugfs_setup_regset32 - Initialize register block definition for debugfs
> - * This function is meant to initialize dpu_debugfs_regset32 structures for use
> - * with dpu_debugfs_create_regset32.
> - * @regset: opaque register definition structure
> - * @offset: sub-block offset
> - * @length: sub-block length, in bytes
> - * @dpu_kms: pointer to dpu kms structure
> - */
> -void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
> -		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms);
> -
>   /**
>    * dpu_debugfs_create_regset32 - Create register read back file for debugfs
>    *
> @@ -195,20 +171,16 @@ void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
>    * names/offsets do not need to be provided. The 'read' function simply outputs
>    * sequential register values over a specified range.
>    *
> - * Similar to the related debugfs_create_regset32 API, the structure pointed to
> - * by regset needs to persist for the lifetime of the created file. The calling
> - * code is responsible for initialization/management of this structure.
> - *
> - * The structure pointed to by regset is meant to be opaque. Please use
> - * dpu_debugfs_setup_regset32 to initialize it.
> - *
>    * @name:   File name within debugfs
>    * @mode:   File mode within debugfs
>    * @parent: Parent directory entry within debugfs, can be NULL
> - * @regset: Pointer to persistent register block definition
> + * @offset: sub-block offset
> + * @length: sub-block length, in bytes
> + * @dpu_kms: pointer to dpu kms structure
>    */
>   void dpu_debugfs_create_regset32(const char *name, umode_t mode,
> -		void *parent, struct dpu_debugfs_regset32 *regset);
> +		void *parent,
> +		uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms);
>   
>   /**
>    * dpu_debugfs_get_root - Return root directory entry for KMS's debugfs
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index d3176f58708e..ef66af696a40 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -108,11 +108,6 @@ struct dpu_plane {
>   	bool is_virtual;
>   	struct list_head mplane_list;
>   	struct dpu_mdss_cfg *catalog;
> -
> -	/* debugfs related stuff */
> -	struct dpu_debugfs_regset32 debugfs_src;
> -	struct dpu_debugfs_regset32 debugfs_scaler;
> -	struct dpu_debugfs_regset32 debugfs_csc;
>   };
>   
>   static const uint64_t supported_format_modifiers[] = {
> @@ -1379,35 +1374,29 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>   			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
>   
>   	/* add register dump support */
> -	dpu_debugfs_setup_regset32(&pdpu->debugfs_src,
> +	dpu_debugfs_create_regset32("src_blk", 0400,
> +			debugfs_root,
>   			sblk->src_blk.base + cfg->base,
>   			sblk->src_blk.len,
>   			kms);
> -	dpu_debugfs_create_regset32("src_blk", 0400,
> -			debugfs_root, &pdpu->debugfs_src);
>   
>   	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>   			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>   			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
> -			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4)) {
> -		dpu_debugfs_setup_regset32(&pdpu->debugfs_scaler,
> +			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
> +		dpu_debugfs_create_regset32("scaler_blk", 0400,
> +				debugfs_root,
>   				sblk->scaler_blk.base + cfg->base,
>   				sblk->scaler_blk.len,
>   				kms);
> -		dpu_debugfs_create_regset32("scaler_blk", 0400,
> -				debugfs_root,
> -				&pdpu->debugfs_scaler);
> -	}
>   
>   	if (cfg->features & BIT(DPU_SSPP_CSC) ||
> -			cfg->features & BIT(DPU_SSPP_CSC_10BIT)) {
> -		dpu_debugfs_setup_regset32(&pdpu->debugfs_csc,
> +			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
> +		dpu_debugfs_create_regset32("csc_blk", 0400,
> +				debugfs_root,
>   				sblk->csc_blk.base + cfg->base,
>   				sblk->csc_blk.len,
>   				kms);
> -		dpu_debugfs_create_regset32("csc_blk", 0400,
> -				debugfs_root, &pdpu->debugfs_csc);
> -	}
>   
>   	debugfs_create_u32("xin_id",
>   			0400,
> 

[PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Dmitry Baryshkov]

We are preparing to change DPU plane implementation. Move SSPP debugfs
code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 +++------------------
 4 files changed, 84 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index d77eb7da5daf..ae3cf2e4d7d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
 #include "dpu_hw_sspp.h"
 #include "dpu_kms.h"
 
+#include <drm/drm_file.h>
+
 #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
 
 /* DPU_SSPP_SRC */
@@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
 		c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
 }
 
+#ifdef CONFIG_DEBUG_FS
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct dpu_kms *kms, struct dentry *entry)
+{
+	const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
+	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+	struct dentry *debugfs_root;
+	char sspp_name[32];
+
+	snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
+
+	/* create overall sub-directory for the pipe */
+	debugfs_root =
+		debugfs_create_dir(sspp_name, entry);
+
+	/* don't error check these */
+	debugfs_create_xul("features", 0600,
+			debugfs_root, (unsigned long *)&hw_pipe->cap->features);
+
+	/* add register dump support */
+	dpu_debugfs_create_regset32("src_blk", 0400,
+			debugfs_root,
+			sblk->src_blk.base + cfg->base,
+			sblk->src_blk.len,
+			kms);
+
+	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
+			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
+			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
+			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+		dpu_debugfs_create_regset32("scaler_blk", 0400,
+				debugfs_root,
+				sblk->scaler_blk.base + cfg->base,
+				sblk->scaler_blk.len,
+				kms);
+
+	if (cfg->features & BIT(DPU_SSPP_CSC) ||
+			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
+		dpu_debugfs_create_regset32("csc_blk", 0400,
+				debugfs_root,
+				sblk->csc_blk.base + cfg->base,
+				sblk->csc_blk.len,
+				kms);
+
+	debugfs_create_u32("xin_id",
+			0400,
+			debugfs_root,
+			(u32 *) &cfg->xin_id);
+	debugfs_create_u32("clk_ctrl",
+			0400,
+			debugfs_root,
+			(u32 *) &cfg->clk_ctrl);
+	debugfs_create_x32("creq_vblank",
+			0600,
+			debugfs_root,
+			(u32 *) &sblk->creq_vblank);
+	debugfs_create_x32("danger_vblank",
+			0600,
+			debugfs_root,
+			(u32 *) &sblk->danger_vblank);
+
+	return 0;
+}
+#endif
+
+
 static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
 		void __iomem *addr,
 		struct dpu_mdss_cfg *catalog,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index e8939d7387cb..cef281687bab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -381,6 +381,7 @@ struct dpu_hw_pipe {
 	struct dpu_hw_sspp_ops ops;
 };
 
+struct dpu_kms;
 /**
  * dpu_hw_sspp_init - initializes the sspp hw driver object.
  * Should be called once before accessing every pipe.
@@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp idx,
  */
 void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
 
+void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root);
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct dpu_kms *kms, struct dentry *entry);
+
 #endif /*_DPU_HW_SSPP_H */
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7e7a619769a8..de9efe6dcf7c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
 	dpu_debugfs_danger_init(dpu_kms, entry);
 	dpu_debugfs_vbif_init(dpu_kms, entry);
 	dpu_debugfs_core_irq_init(dpu_kms, entry);
+	dpu_debugfs_sspp_init(dpu_kms, entry);
 
 	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
 		if (priv->dp[i])
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ef66af696a40..cc7a7eb84fdd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -13,7 +13,6 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_damage_helper.h>
-#include <drm/drm_file.h>
 #include <drm/drm_gem_atomic_helper.h>
 
 #include "msm_drv.h"
@@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
-static int _dpu_plane_init_debugfs(struct drm_plane *plane)
+/* SSPP live inside dpu_plane private data only. Enumerate them here. */
+void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root)
 {
-	struct dpu_plane *pdpu = to_dpu_plane(plane);
-	struct dpu_kms *kms = _dpu_plane_get_kms(plane);
-	const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
-	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
-	struct dentry *debugfs_root;
-
-	/* create overall sub-directory for the pipe */
-	debugfs_root =
-		debugfs_create_dir(plane->name,
-				plane->dev->primary->debugfs_root);
-
-	/* don't error check these */
-	debugfs_create_xul("features", 0600,
-			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
-
-	/* add register dump support */
-	dpu_debugfs_create_regset32("src_blk", 0400,
-			debugfs_root,
-			sblk->src_blk.base + cfg->base,
-			sblk->src_blk.len,
-			kms);
-
-	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
-			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
-			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
-			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
-		dpu_debugfs_create_regset32("scaler_blk", 0400,
-				debugfs_root,
-				sblk->scaler_blk.base + cfg->base,
-				sblk->scaler_blk.len,
-				kms);
-
-	if (cfg->features & BIT(DPU_SSPP_CSC) ||
-			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
-		dpu_debugfs_create_regset32("csc_blk", 0400,
-				debugfs_root,
-				sblk->csc_blk.base + cfg->base,
-				sblk->csc_blk.len,
-				kms);
-
-	debugfs_create_u32("xin_id",
-			0400,
-			debugfs_root,
-			(u32 *) &cfg->xin_id);
-	debugfs_create_u32("clk_ctrl",
-			0400,
-			debugfs_root,
-			(u32 *) &cfg->clk_ctrl);
-	debugfs_create_x32("creq_vblank",
-			0600,
-			debugfs_root,
-			(u32 *) &sblk->creq_vblank);
-	debugfs_create_x32("danger_vblank",
-			0600,
-			debugfs_root,
-			(u32 *) &sblk->danger_vblank);
+	struct drm_plane *plane;
+	struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
 
-	return 0;
-}
-#else
-static int _dpu_plane_init_debugfs(struct drm_plane *plane)
-{
-	return 0;
-}
-#endif
+	if (IS_ERR(entry))
+		return;
 
-static int dpu_plane_late_register(struct drm_plane *plane)
-{
-	return _dpu_plane_init_debugfs(plane);
+	drm_for_each_plane(plane, dpu_kms->dev) {
+		struct dpu_plane *pdpu = to_dpu_plane(plane);
+
+		_dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
+	}
 }
+#endif
 
 static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
 		uint32_t format, uint64_t modifier)
@@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs dpu_plane_funcs = {
 		.reset = dpu_plane_reset,
 		.atomic_duplicate_state = dpu_plane_duplicate_state,
 		.atomic_destroy_state = dpu_plane_destroy_state,
-		.late_register = dpu_plane_late_register,
 		.format_mod_supported = dpu_plane_format_mod_supported,
 };
 
-- 
2.33.0

Re: [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Abhinav Kumar]

On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> We are preparing to change DPU plane implementation. Move SSPP debugfs
> code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 +++------------------
>   4 files changed, 84 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> index d77eb7da5daf..ae3cf2e4d7d9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> @@ -8,6 +8,8 @@
>   #include "dpu_hw_sspp.h"
>   #include "dpu_kms.h"
>   
> +#include <drm/drm_file.h>
> +
>   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>   
>   /* DPU_SSPP_SRC */
> @@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
>   		c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
>   }
>   
> +#ifdef CONFIG_DEBUG_FS
> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct dpu_kms *kms, struct dentry *entry)
> +{
> +	const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
> +	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
> +	struct dentry *debugfs_root;
> +	char sspp_name[32];
> +
> +	snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
> +
> +	/* create overall sub-directory for the pipe */
> +	debugfs_root =
> +		debugfs_create_dir(sspp_name, entry);


I would like to take a different approach to this. Let me know what you 
think.

Let the directory names still be the drm plane names as someone who 
would first get the DRM state and then try to lookup the register values 
of that plane would not know where to look now.

Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an 
extra entry which tells the sspp_index.

This will also establish the plane to SSPP mapping.

Now when planes go virtual in the future, this will be helpful even more
so that we can know the plane to SSPP mapping.


> +
> +	/* don't error check these */
> +	debugfs_create_xul("features", 0600,
> +			debugfs_root, (unsigned long *)&hw_pipe->cap->features);
> +
> +	/* add register dump support */
> +	dpu_debugfs_create_regset32("src_blk", 0400,
> +			debugfs_root,
> +			sblk->src_blk.base + cfg->base,
> +			sblk->src_blk.len,
> +			kms);
> +
> +	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
> +			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
> +			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
> +			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
> +		dpu_debugfs_create_regset32("scaler_blk", 0400,
> +				debugfs_root,
> +				sblk->scaler_blk.base + cfg->base,
> +				sblk->scaler_blk.len,
> +				kms);
> +
> +	if (cfg->features & BIT(DPU_SSPP_CSC) ||
> +			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
> +		dpu_debugfs_create_regset32("csc_blk", 0400,
> +				debugfs_root,
> +				sblk->csc_blk.base + cfg->base,
> +				sblk->csc_blk.len,
> +				kms);
> +
> +	debugfs_create_u32("xin_id",
> +			0400,
> +			debugfs_root,
> +			(u32 *) &cfg->xin_id);
> +	debugfs_create_u32("clk_ctrl",
> +			0400,
> +			debugfs_root,
> +			(u32 *) &cfg->clk_ctrl);
> +	debugfs_create_x32("creq_vblank",
> +			0600,
> +			debugfs_root,
> +			(u32 *) &sblk->creq_vblank);
> +	debugfs_create_x32("danger_vblank",
> +			0600,
> +			debugfs_root,
> +			(u32 *) &sblk->danger_vblank);
> +
> +	return 0;
> +}
> +#endif
> +
> +
>   static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>   		void __iomem *addr,
>   		struct dpu_mdss_cfg *catalog,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index e8939d7387cb..cef281687bab 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -381,6 +381,7 @@ struct dpu_hw_pipe {
>   	struct dpu_hw_sspp_ops ops;
>   };
>   
> +struct dpu_kms;
>   /**
>    * dpu_hw_sspp_init - initializes the sspp hw driver object.
>    * Should be called once before accessing every pipe.
> @@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp idx,
>    */
>   void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
>   
> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root);
> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct dpu_kms *kms, struct dentry *entry);
> +
>   #endif /*_DPU_HW_SSPP_H */
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7e7a619769a8..de9efe6dcf7c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
>   	dpu_debugfs_danger_init(dpu_kms, entry);
>   	dpu_debugfs_vbif_init(dpu_kms, entry);
>   	dpu_debugfs_core_irq_init(dpu_kms, entry);
> +	dpu_debugfs_sspp_init(dpu_kms, entry);
>   
>   	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
>   		if (priv->dp[i])
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index ef66af696a40..cc7a7eb84fdd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -13,7 +13,6 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_uapi.h>
>   #include <drm/drm_damage_helper.h>
> -#include <drm/drm_file.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   
>   #include "msm_drv.h"
> @@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
>   
> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
> +/* SSPP live inside dpu_plane private data only. Enumerate them here. */
> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root)
>   {
> -	struct dpu_plane *pdpu = to_dpu_plane(plane);
> -	struct dpu_kms *kms = _dpu_plane_get_kms(plane);
> -	const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
> -	const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
> -	struct dentry *debugfs_root;
> -
> -	/* create overall sub-directory for the pipe */
> -	debugfs_root =
> -		debugfs_create_dir(plane->name,
> -				plane->dev->primary->debugfs_root);
> -
> -	/* don't error check these */
> -	debugfs_create_xul("features", 0600,
> -			debugfs_root, (unsigned long *)&pdpu->pipe_hw->cap->features);
> -
> -	/* add register dump support */
> -	dpu_debugfs_create_regset32("src_blk", 0400,
> -			debugfs_root,
> -			sblk->src_blk.base + cfg->base,
> -			sblk->src_blk.len,
> -			kms);
> -
> -	if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
> -			cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
> -			cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
> -			cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
> -		dpu_debugfs_create_regset32("scaler_blk", 0400,
> -				debugfs_root,
> -				sblk->scaler_blk.base + cfg->base,
> -				sblk->scaler_blk.len,
> -				kms);
> -
> -	if (cfg->features & BIT(DPU_SSPP_CSC) ||
> -			cfg->features & BIT(DPU_SSPP_CSC_10BIT))
> -		dpu_debugfs_create_regset32("csc_blk", 0400,
> -				debugfs_root,
> -				sblk->csc_blk.base + cfg->base,
> -				sblk->csc_blk.len,
> -				kms);
> -
> -	debugfs_create_u32("xin_id",
> -			0400,
> -			debugfs_root,
> -			(u32 *) &cfg->xin_id);
> -	debugfs_create_u32("clk_ctrl",
> -			0400,
> -			debugfs_root,
> -			(u32 *) &cfg->clk_ctrl);
> -	debugfs_create_x32("creq_vblank",
> -			0600,
> -			debugfs_root,
> -			(u32 *) &sblk->creq_vblank);
> -	debugfs_create_x32("danger_vblank",
> -			0600,
> -			debugfs_root,
> -			(u32 *) &sblk->danger_vblank);
> +	struct drm_plane *plane;
> +	struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
>   
> -	return 0;
> -}
> -#else
> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
> -{
> -	return 0;
> -}
> -#endif
> +	if (IS_ERR(entry))
> +		return;
>   
> -static int dpu_plane_late_register(struct drm_plane *plane)
> -{
> -	return _dpu_plane_init_debugfs(plane);
> +	drm_for_each_plane(plane, dpu_kms->dev) {
> +		struct dpu_plane *pdpu = to_dpu_plane(plane);
> +
> +		_dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
> +	}
>   }
> +#endif
>   
>   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
>   		uint32_t format, uint64_t modifier)
> @@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs dpu_plane_funcs = {
>   		.reset = dpu_plane_reset,
>   		.atomic_duplicate_state = dpu_plane_duplicate_state,
>   		.atomic_destroy_state = dpu_plane_destroy_state,
> -		.late_register = dpu_plane_late_register,
>   		.format_mod_supported = dpu_plane_format_mod_supported,
>   };
>   
> 

Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Abhinav Kumar]

On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
> 
> 
> On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
>> We are preparing to change DPU plane implementation. Move SSPP debugfs
>> code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 +++------------------
>>   4 files changed, 84 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>> index d77eb7da5daf..ae3cf2e4d7d9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>> @@ -8,6 +8,8 @@
>>   #include "dpu_hw_sspp.h"
>>   #include "dpu_kms.h"
>> +#include <drm/drm_file.h>
>> +
>>   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>   /* DPU_SSPP_SRC */
>> @@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
>>           c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
>>   }
>> +#ifdef CONFIG_DEBUG_FS
>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>> dpu_kms *kms, struct dentry *entry)
>> +{
>> +    const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
>> +    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>> +    struct dentry *debugfs_root;
>> +    char sspp_name[32];
>> +
>> +    snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
>> +
>> +    /* create overall sub-directory for the pipe */
>> +    debugfs_root =
>> +        debugfs_create_dir(sspp_name, entry);
> 
> 
> I would like to take a different approach to this. Let me know what you 
> think.
> 
> Let the directory names still be the drm plane names as someone who 
> would first get the DRM state and then try to lookup the register values 
> of that plane would not know where to look now.
> 
> Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an 
> extra entry which tells the sspp_index.
> 
> This will also establish the plane to SSPP mapping.
> 
> Now when planes go virtual in the future, this will be helpful even more
> so that we can know the plane to SSPP mapping.

OR i like rob's suggestion of implementing the atomic_print_state 
callback which will printout the drm plane to SSPP mapping along with 
this change so that when we look at DRM state, we also know the plane
to SSPP mapping and look in the right SSPP's dir.
> 
> 
>> +
>> +    /* don't error check these */
>> +    debugfs_create_xul("features", 0600,
>> +            debugfs_root, (unsigned long *)&hw_pipe->cap->features);
>> +
>> +    /* add register dump support */
>> +    dpu_debugfs_create_regset32("src_blk", 0400,
>> +            debugfs_root,
>> +            sblk->src_blk.base + cfg->base,
>> +            sblk->src_blk.len,
>> +            kms);
>> +
>> +    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>> +        dpu_debugfs_create_regset32("scaler_blk", 0400,
>> +                debugfs_root,
>> +                sblk->scaler_blk.base + cfg->base,
>> +                sblk->scaler_blk.len,
>> +                kms);
>> +
>> +    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>> +            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>> +        dpu_debugfs_create_regset32("csc_blk", 0400,
>> +                debugfs_root,
>> +                sblk->csc_blk.base + cfg->base,
>> +                sblk->csc_blk.len,
>> +                kms);
>> +
>> +    debugfs_create_u32("xin_id",
>> +            0400,
>> +            debugfs_root,
>> +            (u32 *) &cfg->xin_id);
>> +    debugfs_create_u32("clk_ctrl",
>> +            0400,
>> +            debugfs_root,
>> +            (u32 *) &cfg->clk_ctrl);
>> +    debugfs_create_x32("creq_vblank",
>> +            0600,
>> +            debugfs_root,
>> +            (u32 *) &sblk->creq_vblank);
>> +    debugfs_create_x32("danger_vblank",
>> +            0600,
>> +            debugfs_root,
>> +            (u32 *) &sblk->danger_vblank);
>> +
>> +    return 0;
>> +}
>> +#endif
>> +
>> +
>>   static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>>           void __iomem *addr,
>>           struct dpu_mdss_cfg *catalog,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> index e8939d7387cb..cef281687bab 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> @@ -381,6 +381,7 @@ struct dpu_hw_pipe {
>>       struct dpu_hw_sspp_ops ops;
>>   };
>> +struct dpu_kms;
>>   /**
>>    * dpu_hw_sspp_init - initializes the sspp hw driver object.
>>    * Should be called once before accessing every pipe.
>> @@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp 
>> idx,
>>    */
>>   void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>> *debugfs_root);
>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>> dpu_kms *kms, struct dentry *entry);
>> +
>>   #endif /*_DPU_HW_SSPP_H */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 7e7a619769a8..de9efe6dcf7c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms 
>> *kms, struct drm_minor *minor)
>>       dpu_debugfs_danger_init(dpu_kms, entry);
>>       dpu_debugfs_vbif_init(dpu_kms, entry);
>>       dpu_debugfs_core_irq_init(dpu_kms, entry);
>> +    dpu_debugfs_sspp_init(dpu_kms, entry);
>>       for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
>>           if (priv->dp[i])
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index ef66af696a40..cc7a7eb84fdd 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -13,7 +13,6 @@
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_uapi.h>
>>   #include <drm/drm_damage_helper.h>
>> -#include <drm/drm_file.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include "msm_drv.h"
>> @@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct 
>> drm_plane *plane, bool enable)
>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>   }
>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>> +/* SSPP live inside dpu_plane private data only. Enumerate them here. */
>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>> *debugfs_root)
>>   {
>> -    struct dpu_plane *pdpu = to_dpu_plane(plane);
>> -    struct dpu_kms *kms = _dpu_plane_get_kms(plane);
>> -    const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
>> -    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>> -    struct dentry *debugfs_root;
>> -
>> -    /* create overall sub-directory for the pipe */
>> -    debugfs_root =
>> -        debugfs_create_dir(plane->name,
>> -                plane->dev->primary->debugfs_root);
>> -
>> -    /* don't error check these */
>> -    debugfs_create_xul("features", 0600,
>> -            debugfs_root, (unsigned long 
>> *)&pdpu->pipe_hw->cap->features);
>> -
>> -    /* add register dump support */
>> -    dpu_debugfs_create_regset32("src_blk", 0400,
>> -            debugfs_root,
>> -            sblk->src_blk.base + cfg->base,
>> -            sblk->src_blk.len,
>> -            kms);
>> -
>> -    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>> -        dpu_debugfs_create_regset32("scaler_blk", 0400,
>> -                debugfs_root,
>> -                sblk->scaler_blk.base + cfg->base,
>> -                sblk->scaler_blk.len,
>> -                kms);
>> -
>> -    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>> -            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>> -        dpu_debugfs_create_regset32("csc_blk", 0400,
>> -                debugfs_root,
>> -                sblk->csc_blk.base + cfg->base,
>> -                sblk->csc_blk.len,
>> -                kms);
>> -
>> -    debugfs_create_u32("xin_id",
>> -            0400,
>> -            debugfs_root,
>> -            (u32 *) &cfg->xin_id);
>> -    debugfs_create_u32("clk_ctrl",
>> -            0400,
>> -            debugfs_root,
>> -            (u32 *) &cfg->clk_ctrl);
>> -    debugfs_create_x32("creq_vblank",
>> -            0600,
>> -            debugfs_root,
>> -            (u32 *) &sblk->creq_vblank);
>> -    debugfs_create_x32("danger_vblank",
>> -            0600,
>> -            debugfs_root,
>> -            (u32 *) &sblk->danger_vblank);
>> +    struct drm_plane *plane;
>> +    struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
>> -    return 0;
>> -}
>> -#else
>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>> -{
>> -    return 0;
>> -}
>> -#endif
>> +    if (IS_ERR(entry))
>> +        return;
>> -static int dpu_plane_late_register(struct drm_plane *plane)
>> -{
>> -    return _dpu_plane_init_debugfs(plane);
>> +    drm_for_each_plane(plane, dpu_kms->dev) {
>> +        struct dpu_plane *pdpu = to_dpu_plane(plane);
>> +
>> +        _dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
>> +    }
>>   }
>> +#endif
>>   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
>>           uint32_t format, uint64_t modifier)
>> @@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs 
>> dpu_plane_funcs = {
>>           .reset = dpu_plane_reset,
>>           .atomic_duplicate_state = dpu_plane_duplicate_state,
>>           .atomic_destroy_state = dpu_plane_destroy_state,
>> -        .late_register = dpu_plane_late_register,
>>           .format_mod_supported = dpu_plane_format_mod_supported,
>>   };
>>

Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Dmitry Baryshkov]

On 10/12/2021 01:27, Abhinav Kumar wrote:
> 
> 
> On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
>>
>>
>> On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
>>> We are preparing to change DPU plane implementation. Move SSPP debugfs
>>> code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 +++------------------
>>>   4 files changed, 84 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> index d77eb7da5daf..ae3cf2e4d7d9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> @@ -8,6 +8,8 @@
>>>   #include "dpu_hw_sspp.h"
>>>   #include "dpu_kms.h"
>>> +#include <drm/drm_file.h>
>>> +
>>>   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>>   /* DPU_SSPP_SRC */
>>> @@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
>>>           c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
>>>   }
>>> +#ifdef CONFIG_DEBUG_FS
>>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>>> dpu_kms *kms, struct dentry *entry)
>>> +{
>>> +    const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
>>> +    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>>> +    struct dentry *debugfs_root;
>>> +    char sspp_name[32];
>>> +
>>> +    snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
>>> +
>>> +    /* create overall sub-directory for the pipe */
>>> +    debugfs_root =
>>> +        debugfs_create_dir(sspp_name, entry);
>>
>>
>> I would like to take a different approach to this. Let me know what 
>> you think.
>>
>> Let the directory names still be the drm plane names as someone who 
>> would first get the DRM state and then try to lookup the register 
>> values of that plane would not know where to look now.
>>
>> Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an 
>> extra entry which tells the sspp_index.
>>
>> This will also establish the plane to SSPP mapping.
>>
>> Now when planes go virtual in the future, this will be helpful even more
>> so that we can know the plane to SSPP mapping.
> 
> OR i like rob's suggestion of implementing the atomic_print_state 
> callback which will printout the drm plane to SSPP mapping along with 
> this change so that when we look at DRM state, we also know the plane
> to SSPP mapping and look in the right SSPP's dir.

I'd add atomic_print_state(), it seems simpler (and more future-proof).

>>
>>
>>> +
>>> +    /* don't error check these */
>>> +    debugfs_create_xul("features", 0600,
>>> +            debugfs_root, (unsigned long *)&hw_pipe->cap->features);
>>> +
>>> +    /* add register dump support */
>>> +    dpu_debugfs_create_regset32("src_blk", 0400,
>>> +            debugfs_root,
>>> +            sblk->src_blk.base + cfg->base,
>>> +            sblk->src_blk.len,
>>> +            kms);
>>> +
>>> +    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>> +        dpu_debugfs_create_regset32("scaler_blk", 0400,
>>> +                debugfs_root,
>>> +                sblk->scaler_blk.base + cfg->base,
>>> +                sblk->scaler_blk.len,
>>> +                kms);
>>> +
>>> +    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>>> +            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>>> +        dpu_debugfs_create_regset32("csc_blk", 0400,
>>> +                debugfs_root,
>>> +                sblk->csc_blk.base + cfg->base,
>>> +                sblk->csc_blk.len,
>>> +                kms);
>>> +
>>> +    debugfs_create_u32("xin_id",
>>> +            0400,
>>> +            debugfs_root,
>>> +            (u32 *) &cfg->xin_id);
>>> +    debugfs_create_u32("clk_ctrl",
>>> +            0400,
>>> +            debugfs_root,
>>> +            (u32 *) &cfg->clk_ctrl);
>>> +    debugfs_create_x32("creq_vblank",
>>> +            0600,
>>> +            debugfs_root,
>>> +            (u32 *) &sblk->creq_vblank);
>>> +    debugfs_create_x32("danger_vblank",
>>> +            0600,
>>> +            debugfs_root,
>>> +            (u32 *) &sblk->danger_vblank);
>>> +
>>> +    return 0;
>>> +}
>>> +#endif
>>> +
>>> +
>>>   static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>>>           void __iomem *addr,
>>>           struct dpu_mdss_cfg *catalog,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> index e8939d7387cb..cef281687bab 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> @@ -381,6 +381,7 @@ struct dpu_hw_pipe {
>>>       struct dpu_hw_sspp_ops ops;
>>>   };
>>> +struct dpu_kms;
>>>   /**
>>>    * dpu_hw_sspp_init - initializes the sspp hw driver object.
>>>    * Should be called once before accessing every pipe.
>>> @@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum 
>>> dpu_sspp idx,
>>>    */
>>>   void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
>>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>>> *debugfs_root);
>>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>>> dpu_kms *kms, struct dentry *entry);
>>> +
>>>   #endif /*_DPU_HW_SSPP_H */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 7e7a619769a8..de9efe6dcf7c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms 
>>> *kms, struct drm_minor *minor)
>>>       dpu_debugfs_danger_init(dpu_kms, entry);
>>>       dpu_debugfs_vbif_init(dpu_kms, entry);
>>>       dpu_debugfs_core_irq_init(dpu_kms, entry);
>>> +    dpu_debugfs_sspp_init(dpu_kms, entry);
>>>       for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
>>>           if (priv->dp[i])
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index ef66af696a40..cc7a7eb84fdd 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -13,7 +13,6 @@
>>>   #include <drm/drm_atomic.h>
>>>   #include <drm/drm_atomic_uapi.h>
>>>   #include <drm/drm_damage_helper.h>
>>> -#include <drm/drm_file.h>
>>>   #include <drm/drm_gem_atomic_helper.h>
>>>   #include "msm_drv.h"
>>> @@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct 
>>> drm_plane *plane, bool enable)
>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>   }
>>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>>> +/* SSPP live inside dpu_plane private data only. Enumerate them 
>>> here. */
>>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>>> *debugfs_root)
>>>   {
>>> -    struct dpu_plane *pdpu = to_dpu_plane(plane);
>>> -    struct dpu_kms *kms = _dpu_plane_get_kms(plane);
>>> -    const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
>>> -    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>>> -    struct dentry *debugfs_root;
>>> -
>>> -    /* create overall sub-directory for the pipe */
>>> -    debugfs_root =
>>> -        debugfs_create_dir(plane->name,
>>> -                plane->dev->primary->debugfs_root);
>>> -
>>> -    /* don't error check these */
>>> -    debugfs_create_xul("features", 0600,
>>> -            debugfs_root, (unsigned long 
>>> *)&pdpu->pipe_hw->cap->features);
>>> -
>>> -    /* add register dump support */
>>> -    dpu_debugfs_create_regset32("src_blk", 0400,
>>> -            debugfs_root,
>>> -            sblk->src_blk.base + cfg->base,
>>> -            sblk->src_blk.len,
>>> -            kms);
>>> -
>>> -    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>> -        dpu_debugfs_create_regset32("scaler_blk", 0400,
>>> -                debugfs_root,
>>> -                sblk->scaler_blk.base + cfg->base,
>>> -                sblk->scaler_blk.len,
>>> -                kms);
>>> -
>>> -    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>>> -            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>>> -        dpu_debugfs_create_regset32("csc_blk", 0400,
>>> -                debugfs_root,
>>> -                sblk->csc_blk.base + cfg->base,
>>> -                sblk->csc_blk.len,
>>> -                kms);
>>> -
>>> -    debugfs_create_u32("xin_id",
>>> -            0400,
>>> -            debugfs_root,
>>> -            (u32 *) &cfg->xin_id);
>>> -    debugfs_create_u32("clk_ctrl",
>>> -            0400,
>>> -            debugfs_root,
>>> -            (u32 *) &cfg->clk_ctrl);
>>> -    debugfs_create_x32("creq_vblank",
>>> -            0600,
>>> -            debugfs_root,
>>> -            (u32 *) &sblk->creq_vblank);
>>> -    debugfs_create_x32("danger_vblank",
>>> -            0600,
>>> -            debugfs_root,
>>> -            (u32 *) &sblk->danger_vblank);
>>> +    struct drm_plane *plane;
>>> +    struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
>>> -    return 0;
>>> -}
>>> -#else
>>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>>> -{
>>> -    return 0;
>>> -}
>>> -#endif
>>> +    if (IS_ERR(entry))
>>> +        return;
>>> -static int dpu_plane_late_register(struct drm_plane *plane)
>>> -{
>>> -    return _dpu_plane_init_debugfs(plane);
>>> +    drm_for_each_plane(plane, dpu_kms->dev) {
>>> +        struct dpu_plane *pdpu = to_dpu_plane(plane);
>>> +
>>> +        _dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
>>> +    }
>>>   }
>>> +#endif
>>>   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
>>>           uint32_t format, uint64_t modifier)
>>> @@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs 
>>> dpu_plane_funcs = {
>>>           .reset = dpu_plane_reset,
>>>           .atomic_duplicate_state = dpu_plane_duplicate_state,
>>>           .atomic_destroy_state = dpu_plane_destroy_state,
>>> -        .late_register = dpu_plane_late_register,
>>>           .format_mod_supported = dpu_plane_format_mod_supported,
>>>   };
>>>


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Abhinav Kumar]

On 12/9/2021 4:19 PM, Dmitry Baryshkov wrote:
> On 10/12/2021 01:27, Abhinav Kumar wrote:
>>
>>
>> On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
>>>> We are preparing to change DPU plane implementation. Move SSPP debugfs
>>>> code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 
>>>> +++------------------
>>>>   4 files changed, 84 insertions(+), 70 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> index d77eb7da5daf..ae3cf2e4d7d9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> @@ -8,6 +8,8 @@
>>>>   #include "dpu_hw_sspp.h"
>>>>   #include "dpu_kms.h"
>>>> +#include <drm/drm_file.h>
>>>> +
>>>>   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>>>   /* DPU_SSPP_SRC */
>>>> @@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe 
>>>> *c,
>>>>           c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
>>>>   }
>>>> +#ifdef CONFIG_DEBUG_FS
>>>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>>>> dpu_kms *kms, struct dentry *entry)
>>>> +{
>>>> +    const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
>>>> +    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>>>> +    struct dentry *debugfs_root;
>>>> +    char sspp_name[32];
>>>> +
>>>> +    snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
>>>> +
>>>> +    /* create overall sub-directory for the pipe */
>>>> +    debugfs_root =
>>>> +        debugfs_create_dir(sspp_name, entry);
>>>
>>>
>>> I would like to take a different approach to this. Let me know what 
>>> you think.
>>>
>>> Let the directory names still be the drm plane names as someone who 
>>> would first get the DRM state and then try to lookup the register 
>>> values of that plane would not know where to look now.
>>>
>>> Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an 
>>> extra entry which tells the sspp_index.
>>>
>>> This will also establish the plane to SSPP mapping.
>>>
>>> Now when planes go virtual in the future, this will be helpful even more
>>> so that we can know the plane to SSPP mapping.
>>
>> OR i like rob's suggestion of implementing the atomic_print_state 
>> callback which will printout the drm plane to SSPP mapping along with 
>> this change so that when we look at DRM state, we also know the plane
>> to SSPP mapping and look in the right SSPP's dir.
> 
> I'd add atomic_print_state(), it seems simpler (and more future-proof).
Now, that https://patchwork.freedesktop.org/patch/467031/ has been pushed,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
>>>
>>>
>>>> +
>>>> +    /* don't error check these */
>>>> +    debugfs_create_xul("features", 0600,
>>>> +            debugfs_root, (unsigned long *)&hw_pipe->cap->features);
>>>> +
>>>> +    /* add register dump support */
>>>> +    dpu_debugfs_create_regset32("src_blk", 0400,
>>>> +            debugfs_root,
>>>> +            sblk->src_blk.base + cfg->base,
>>>> +            sblk->src_blk.len,
>>>> +            kms);
>>>> +
>>>> +    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>>>> +            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>> +        dpu_debugfs_create_regset32("scaler_blk", 0400,
>>>> +                debugfs_root,
>>>> +                sblk->scaler_blk.base + cfg->base,
>>>> +                sblk->scaler_blk.len,
>>>> +                kms);
>>>> +
>>>> +    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>>>> +            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>>>> +        dpu_debugfs_create_regset32("csc_blk", 0400,
>>>> +                debugfs_root,
>>>> +                sblk->csc_blk.base + cfg->base,
>>>> +                sblk->csc_blk.len,
>>>> +                kms);
>>>> +
>>>> +    debugfs_create_u32("xin_id",
>>>> +            0400,
>>>> +            debugfs_root,
>>>> +            (u32 *) &cfg->xin_id);
>>>> +    debugfs_create_u32("clk_ctrl",
>>>> +            0400,
>>>> +            debugfs_root,
>>>> +            (u32 *) &cfg->clk_ctrl);
>>>> +    debugfs_create_x32("creq_vblank",
>>>> +            0600,
>>>> +            debugfs_root,
>>>> +            (u32 *) &sblk->creq_vblank);
>>>> +    debugfs_create_x32("danger_vblank",
>>>> +            0600,
>>>> +            debugfs_root,
>>>> +            (u32 *) &sblk->danger_vblank);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +
>>>>   static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>>>>           void __iomem *addr,
>>>>           struct dpu_mdss_cfg *catalog,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> index e8939d7387cb..cef281687bab 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> @@ -381,6 +381,7 @@ struct dpu_hw_pipe {
>>>>       struct dpu_hw_sspp_ops ops;
>>>>   };
>>>> +struct dpu_kms;
>>>>   /**
>>>>    * dpu_hw_sspp_init - initializes the sspp hw driver object.
>>>>    * Should be called once before accessing every pipe.
>>>> @@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum 
>>>> dpu_sspp idx,
>>>>    */
>>>>   void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
>>>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>>>> *debugfs_root);
>>>> +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
>>>> dpu_kms *kms, struct dentry *entry);
>>>> +
>>>>   #endif /*_DPU_HW_SSPP_H */
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 7e7a619769a8..de9efe6dcf7c 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms 
>>>> *kms, struct drm_minor *minor)
>>>>       dpu_debugfs_danger_init(dpu_kms, entry);
>>>>       dpu_debugfs_vbif_init(dpu_kms, entry);
>>>>       dpu_debugfs_core_irq_init(dpu_kms, entry);
>>>> +    dpu_debugfs_sspp_init(dpu_kms, entry);
>>>>       for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
>>>>           if (priv->dp[i])
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> index ef66af696a40..cc7a7eb84fdd 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> @@ -13,7 +13,6 @@
>>>>   #include <drm/drm_atomic.h>
>>>>   #include <drm/drm_atomic_uapi.h>
>>>>   #include <drm/drm_damage_helper.h>
>>>> -#include <drm/drm_file.h>
>>>>   #include <drm/drm_gem_atomic_helper.h>
>>>>   #include "msm_drv.h"
>>>> @@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct 
>>>> drm_plane *plane, bool enable)
>>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>>   }
>>>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>>>> +/* SSPP live inside dpu_plane private data only. Enumerate them 
>>>> here. */
>>>> +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
>>>> *debugfs_root)
>>>>   {
>>>> -    struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>> -    struct dpu_kms *kms = _dpu_plane_get_kms(plane);
>>>> -    const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
>>>> -    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
>>>> -    struct dentry *debugfs_root;
>>>> -
>>>> -    /* create overall sub-directory for the pipe */
>>>> -    debugfs_root =
>>>> -        debugfs_create_dir(plane->name,
>>>> -                plane->dev->primary->debugfs_root);
>>>> -
>>>> -    /* don't error check these */
>>>> -    debugfs_create_xul("features", 0600,
>>>> -            debugfs_root, (unsigned long 
>>>> *)&pdpu->pipe_hw->cap->features);
>>>> -
>>>> -    /* add register dump support */
>>>> -    dpu_debugfs_create_regset32("src_blk", 0400,
>>>> -            debugfs_root,
>>>> -            sblk->src_blk.base + cfg->base,
>>>> -            sblk->src_blk.len,
>>>> -            kms);
>>>> -
>>>> -    if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
>>>> -            cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>> -        dpu_debugfs_create_regset32("scaler_blk", 0400,
>>>> -                debugfs_root,
>>>> -                sblk->scaler_blk.base + cfg->base,
>>>> -                sblk->scaler_blk.len,
>>>> -                kms);
>>>> -
>>>> -    if (cfg->features & BIT(DPU_SSPP_CSC) ||
>>>> -            cfg->features & BIT(DPU_SSPP_CSC_10BIT))
>>>> -        dpu_debugfs_create_regset32("csc_blk", 0400,
>>>> -                debugfs_root,
>>>> -                sblk->csc_blk.base + cfg->base,
>>>> -                sblk->csc_blk.len,
>>>> -                kms);
>>>> -
>>>> -    debugfs_create_u32("xin_id",
>>>> -            0400,
>>>> -            debugfs_root,
>>>> -            (u32 *) &cfg->xin_id);
>>>> -    debugfs_create_u32("clk_ctrl",
>>>> -            0400,
>>>> -            debugfs_root,
>>>> -            (u32 *) &cfg->clk_ctrl);
>>>> -    debugfs_create_x32("creq_vblank",
>>>> -            0600,
>>>> -            debugfs_root,
>>>> -            (u32 *) &sblk->creq_vblank);
>>>> -    debugfs_create_x32("danger_vblank",
>>>> -            0600,
>>>> -            debugfs_root,
>>>> -            (u32 *) &sblk->danger_vblank);
>>>> +    struct drm_plane *plane;
>>>> +    struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
>>>> -    return 0;
>>>> -}
>>>> -#else
>>>> -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
>>>> -{
>>>> -    return 0;
>>>> -}
>>>> -#endif
>>>> +    if (IS_ERR(entry))
>>>> +        return;
>>>> -static int dpu_plane_late_register(struct drm_plane *plane)
>>>> -{
>>>> -    return _dpu_plane_init_debugfs(plane);
>>>> +    drm_for_each_plane(plane, dpu_kms->dev) {
>>>> +        struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>> +
>>>> +        _dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
>>>> +    }
>>>>   }
>>>> +#endif
>>>>   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
>>>>           uint32_t format, uint64_t modifier)
>>>> @@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs 
>>>> dpu_plane_funcs = {
>>>>           .reset = dpu_plane_reset,
>>>>           .atomic_duplicate_state = dpu_plane_duplicate_state,
>>>>           .atomic_destroy_state = dpu_plane_destroy_state,
>>>> -        .late_register = dpu_plane_late_register,
>>>>           .format_mod_supported = dpu_plane_format_mod_supported,
>>>>   };
>>>>
> 
>