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

[Dmitry Baryshkov]

Hi Abhinav,

On Thu, 2 Feb 2023 at 20:41, Abhinav Kumar <quic_abhinavk at quicinc.com> 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 at linaro.org>
>
> 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.

>
> 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.


>
> > ---
> >   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,



-- 
With best wishes
Dmitry