[PATCH v2 05/27] drm/msm/dpu: drop EAGAIN check from dpu_format_populate_layout

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
> > The pipe's layout is not cached, corresponding data structure is zeroed
> > out each time in the dpu_plane_sspp_atomic_update(), right before the
> > call to _dpu_plane_set_scanout() -> dpu_format_populate_layout().
> >
> > Drop plane_addr comparison against previous layout and corresponding
> > EAGAIN handling.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>
> The change itself LGTM, hence
>
> But, shouldnt we add this EAGAIN validation or in other words fix this
> rather than drop this?

What for? Does it really save us anything? What's the price of
re-programming the SSPP_SRC0_ADDR registers?

>
> Like I wrote in the review last time, this makes sure to fail the commit
> if the same addr is being programmed.

First, there is nothing wrong with committing the same source addr.
For example setting the atomic property incurs an internal
drm_atomic_commit() with no change to addresses at all.
And then, this doesn't make atomic_commit fail. Instead it just
shortcuts a call to SSPP->setup_sourceaddress.

>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +---------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   |  4 +---
> >   2 files changed, 2 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > index d95540309d4d..ec1001e10f4f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > @@ -918,8 +918,7 @@ int dpu_format_populate_layout(
> >               struct drm_framebuffer *fb,
> >               struct dpu_hw_fmt_layout *layout)
> >   {
> > -     uint32_t plane_addr[DPU_MAX_PLANES];
> > -     int i, ret;
> > +     int ret;
> >
> >       if (!fb || !layout) {
> >               DRM_ERROR("invalid arguments\n");
> > @@ -940,9 +939,6 @@ int dpu_format_populate_layout(
> >       if (ret)
> >               return ret;
> >
> > -     for (i = 0; i < DPU_MAX_PLANES; ++i)
> > -             plane_addr[i] = layout->plane_addr[i];
> > -
> >       /* Populate the addresses given the fb */
> >       if (DPU_FORMAT_IS_UBWC(layout->format) ||
> >                       DPU_FORMAT_IS_TILE(layout->format))
> > @@ -950,10 +946,6 @@ int dpu_format_populate_layout(
> >       else
> >               ret = _dpu_format_populate_addrs_linear(aspace, fb, layout);
> >
> > -     /* check if anything changed */
> > -     if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr)))
> > -             ret = -EAGAIN;
> > -
> >       return ret;
> >   }
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index cdde7b9ec882..43fb8e00ada6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane,
> >       int ret;
> >
> >       ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
> > -     if (ret == -EAGAIN)
> > -             DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
> > -     else if (ret)
> > +     if (ret)
> >               DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
> >       else if (pdpu->pipe_hw->ops.setup_sourceaddress) {
> >               trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx,



-- 
With best wishes
Dmitry