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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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.

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



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux