On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
