Re: [PATCH 15/25] drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg

Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> · Sat, 14 May 2022 09:53:45 +0300

On 13/05/2022 21:58, Abhinav Kumar wrote:


On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
Remove dpu_hw_fmt_layout instance from struct dpu_hw_fmt_layout, leaving
only src_rect and dst_rect.

I believe you meant "remove dpu_hw_fmt_layout instance from struct 
dpu_hw_pipe_cfg" ?.

Otherwise nothing wrong with the change as such, but other than making 
struct dpu_hw_pipe_cfg lighter, is there any other motivation behind 
this change?

Dont you think that dpu_hw_fmt_layout remaining to be a part of the 
struct dpu_hw_pipe_cfg is better and they are indeed tied closely.

The main motivation was to allow using dpu_hw_pipe_cfg for either of 
plane SSPPs while keeping the layout intact. See the patches 20 and 25.
Without this change the layout will be duplicated between both configs.




Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++-----------
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  6 ++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   |  7 ++---
  3 files changed, 21 insertions(+), 22 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 2186506e6315..df6698778b6d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct 
dpu_sw_pipe *pipe,
  }
  static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
-        struct dpu_hw_pipe_cfg *cfg)
+        struct dpu_hw_fmt_layout *layout)
  {
      struct dpu_hw_pipe *ctx = pipe->sspp;
      u32 ystride0, ystride1;
@@ -497,41 +497,41 @@ static void 
dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
          return;
      if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) {
-        for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++)
+        for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++)
              DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4,
-                    cfg->layout.plane_addr[i]);
+                    layout->plane_addr[i]);
      } else if (pipe->multirect_index == DPU_SSPP_RECT_0) {
          DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx,
-                cfg->layout.plane_addr[0]);
+                layout->plane_addr[0]);
          DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx,
-                cfg->layout.plane_addr[2]);
+                layout->plane_addr[2]);
      } else {
          DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx,
-                cfg->layout.plane_addr[0]);
+                layout->plane_addr[0]);
          DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
-                cfg->layout.plane_addr[2]);
+                layout->plane_addr[2]);
      }
      if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) {
-        ystride0 = (cfg->layout.plane_pitch[0]) |
-            (cfg->layout.plane_pitch[1] << 16);
-        ystride1 = (cfg->layout.plane_pitch[2]) |
-            (cfg->layout.plane_pitch[3] << 16);
+        ystride0 = (layout->plane_pitch[0]) |
+            (layout->plane_pitch[1] << 16);
+        ystride1 = (layout->plane_pitch[2]) |
+            (layout->plane_pitch[3] << 16);
      } else {
          ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx);
          ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx);
          if (pipe->multirect_index == DPU_SSPP_RECT_0) {
              ystride0 = (ystride0 & 0xFFFF0000) |
-                (cfg->layout.plane_pitch[0] & 0x0000FFFF);
+                (layout->plane_pitch[0] & 0x0000FFFF);
              ystride1 = (ystride1 & 0xFFFF0000)|
-                (cfg->layout.plane_pitch[2] & 0x0000FFFF);
+                (layout->plane_pitch[2] & 0x0000FFFF);
          } else {
              ystride0 = (ystride0 & 0x0000FFFF) |
-                ((cfg->layout.plane_pitch[0] << 16) &
+                ((layout->plane_pitch[0] << 16) &
                   0xFFFF0000);
              ystride1 = (ystride1 & 0x0000FFFF) |
-                ((cfg->layout.plane_pitch[2] << 16) &
+                ((layout->plane_pitch[2] << 16) &
                   0xFFFF0000);
          }
      }
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 eee8501ea80d..93b60545ba98 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -155,13 +155,11 @@ struct dpu_hw_pixel_ext {
  /**
   * struct dpu_hw_pipe_cfg : Pipe description
- * @layout:    format layout information for programming buffer to 
hardware
   * @src_rect:  src ROI, caller takes into account the different 
operations
   *             such as decimation, flip etc to program this field
   * @dest_rect: destination ROI.
   */
  struct dpu_hw_pipe_cfg {
-    struct dpu_hw_fmt_layout layout;
      struct drm_rect src_rect;
      struct drm_rect dst_rect;
  };
@@ -260,10 +258,10 @@ struct dpu_hw_sspp_ops {
      /**
       * setup_sourceaddress - setup pipe source addresses
       * @pipe: Pointer to software pipe context
-     * @cfg: Pointer to pipe config structure
+     * @layout: format layout information for programming buffer to 
hardware
       */
      void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx,
-            struct dpu_hw_pipe_cfg *cfg);
+            struct dpu_hw_fmt_layout *layout);
      /**
       * setup_csc - setup color space coversion
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index e9421fa2fb2e..a521c0681af6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1052,6 +1052,7 @@ static void dpu_plane_sspp_atomic_update(struct 
drm_plane *plane)
      const struct dpu_format *fmt =
          to_dpu_format(msm_framebuffer_format(fb));
      struct dpu_hw_pipe_cfg pipe_cfg;
+    struct dpu_hw_fmt_layout layout;
      struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
      struct msm_gem_address_space *aspace = kms->base.aspace;
      bool update_src_addr = true;
@@ -1059,7 +1060,7 @@ static void dpu_plane_sspp_atomic_update(struct 
drm_plane *plane)
      memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
-    ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg.layout);
+    ret = dpu_format_populate_layout(aspace, fb, &layout);
      if (ret == -EAGAIN) {
          DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
          update_src_addr = false;
@@ -1070,8 +1071,8 @@ static void dpu_plane_sspp_atomic_update(struct 
drm_plane *plane)
      if (update_src_addr &&
          pipe->sspp->ops.setup_sourceaddress) {
-        trace_dpu_plane_set_scanout(pipe, &pipe_cfg.layout);
-        pipe->sspp->ops.setup_sourceaddress(pipe, &pipe_cfg);
+        trace_dpu_plane_set_scanout(pipe, &layout);
+        pipe->sspp->ops.setup_sourceaddress(pipe, &layout);
      }
      pstate->pending = true;


--
With best wishes
Dmitry






