Re: [PATCH v2 11/27] drm/msm/dpu: move stride programming to dpu_hw_sspp_setup_sourceaddress

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Thu, 2 Feb 2023 11:15:28 -0800

On 2/2/2023 10:55 AM, Dmitry Baryshkov wrote:
Hi Abhinav,

On Thu, 2 Feb 2023 at 20:41, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
dpu_hw_sspp_setup_rects() programs only source and destination
rectangles.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

Sorry but once again, I dont see a response to my comment

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

So let me repeat that here:

"This separation is logically correct, but there is another codepath
using this.

_dpu_plane_color_fill() calls pdpu->pipe_hw->ops.setup_rects.

So for solid fill, I presume that stride getting programmed is 0 as
there is no buffer to fetch from.

Could you please verify with the HW team what should be the correct
stride programming for the solid fill? I'll have to check what is
being programmed ATM.


Sure, I can check but in the _dpu_plane_color_fill() method the 
pipe_cfg->layout is not filled up so it should be a 0 stride.


But with this separation, we will miss re-programming stride and it will
remain at the old value even for solid fil cases?

You might want to add setup_sourceaddress call there? But that wont make
sense either because for solid fill there is nothing to fetch from.

Perhaps, another op for stride programming then?
"

Also, this is the second patch in the series where the previous comments
were not resolved/responded to.

Hope that this was not just another rebase without looking at the prior
comments.

I might have missed some of the comments during the rebase, please
excuse me. There was no intent to ignore them.


Ack.



---
   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 57 +++++++++++----------
   1 file changed, 29 insertions(+), 28 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 176cd6dc9a69..2bd39c13d54d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -447,7 +447,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
   {
       struct dpu_hw_sspp *ctx = pipe->sspp;
       struct dpu_hw_blk_reg_map *c;
-     u32 src_size, src_xy, dst_size, dst_xy, ystride0, ystride1;
+     u32 src_size, src_xy, dst_size, dst_xy;
       u32 src_size_off, src_xy_off, out_size_off, out_xy_off;
       u32 idx;

@@ -478,44 +478,18 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
       dst_size = (drm_rect_height(&cfg->dst_rect) << 16) |
               drm_rect_width(&cfg->dst_rect);

-     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);
-     } else {
-             ystride0 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE0 + idx);
-             ystride1 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE1 + idx);
-
-             if (pipe->multirect_index == DPU_SSPP_RECT_0) {
-                     ystride0 = (ystride0 & 0xFFFF0000) |
-                             (cfg->layout.plane_pitch[0] & 0x0000FFFF);
-                     ystride1 = (ystride1 & 0xFFFF0000)|
-                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
-             } else {
-                     ystride0 = (ystride0 & 0x0000FFFF) |
-                             ((cfg->layout.plane_pitch[0] << 16) &
-                              0xFFFF0000);
-                     ystride1 = (ystride1 & 0x0000FFFF) |
-                             ((cfg->layout.plane_pitch[2] << 16) &
-                              0xFFFF0000);
-             }
-     }
-
       /* rectangle register programming */
       DPU_REG_WRITE(c, src_size_off + idx, src_size);
       DPU_REG_WRITE(c, src_xy_off + idx, src_xy);
       DPU_REG_WRITE(c, out_size_off + idx, dst_size);
       DPU_REG_WRITE(c, out_xy_off + idx, dst_xy);
-
-     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE0 + idx, ystride0);
-     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE1 + idx, ystride1);
   }

   static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
               struct dpu_hw_pipe_cfg *cfg)
   {
       struct dpu_hw_sspp *ctx = pipe->sspp;
+     u32 ystride0, ystride1;
       int i;
       u32 idx;

@@ -537,6 +511,33 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
               DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
                               cfg->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);
+     } 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);
+                     ystride1 = (ystride1 & 0xFFFF0000)|
+                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
+             } else {
+                     ystride0 = (ystride0 & 0x0000FFFF) |
+                             ((cfg->layout.plane_pitch[0] << 16) &
+                              0xFFFF0000);
+                     ystride1 = (ystride1 & 0x0000FFFF) |
+                             ((cfg->layout.plane_pitch[2] << 16) &
+                              0xFFFF0000);
+             }
+     }
+
+     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx, ystride0);
+     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx, ystride1);
   }

   static void dpu_hw_sspp_setup_csc(struct dpu_hw_sspp *ctx,









