Re: [PATCH v2 12/27] drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg

[Abhinav Kumar]

On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote:
On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar <quic_abhin...@quicinc.com> wrote:



On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving
only src_rect and dst_rect. This way right and left pipes will have
separate dpu_hw_pipe_cfg isntances, while the layout is common to both
of them.


Sorry for not responding to this comment earlier.

https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370

  From the perspective of wide planes you are right that the layout is
common but not true from smart DMA point of view.

For wide planes, yes, its usually the same buffer with just the src_x
being different but conceptually and even HW wise each rectangle of the
smart DMA is capable of fetching from a different buffer.

  From the pov, this decision of not having the dpu_hw_fmt_layout as part
of dpu_hw_pipe_cfg seems incorrect to me.

Yes, each rectangle/pipe can fetch from a different buffer. However in
our use case the layout is not defined for each pipe. It is defined
for a plane, no matter how many pipes are used for the plane, since
the buffer is also defined per plane.

Even if the layout is defined per plane.

So lets say

plane A with layout A maps to rect 1 of DMA0
plane B with layout B maps to rect 2 of DMA0

How can layout be assumed to be duplicated in this case?

This is not a wide plane use-case but just smartDMA case of two 
different layers.

Maybe I am missing something but this is the example i am interested about.


Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
---
   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   | 10 +++----
   3 files changed, 22 insertions(+), 24 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 2bd39c13d54d..400d043f37fa 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_sspp *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 c713343378aa..8dad52eb2a90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -154,13 +154,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;
   };
@@ -243,10 +241,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 cbff4dea8662..0d2a7170e0ab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane 
*plane)

   static void _dpu_plane_set_scanout(struct drm_plane *plane,
               struct dpu_plane_state *pstate,
-             struct dpu_hw_pipe_cfg *pipe_cfg,
               struct drm_framebuffer *fb)
   {
       struct dpu_plane *pdpu = to_dpu_plane(plane);
       struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
       struct msm_gem_address_space *aspace = kms->base.aspace;
+     struct dpu_hw_fmt_layout layout;
       int ret;

-     ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
+     ret = dpu_format_populate_layout(aspace, fb, &layout);
       if (ret)
               DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
       else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
               trace_dpu_plane_set_scanout(&pstate->pipe,
-                                         &pipe_cfg->layout);
-             pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, 
pipe_cfg);
+                                         &layout);
+             pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, 
&layout);
       }
   }

@@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane 
*plane)

       memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));

-     _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
+     _dpu_plane_set_scanout(plane, pstate, fb);

       pstate->pending = true;