On Fri, Apr 19, 2024 at 07:37:44PM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 6:34 PM, Dmitry Baryshkov wrote:
> > On Fri, Apr 19, 2024 at 05:14:01PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> > > > Move a call to dpu_format_populate_plane_sizes() to the atomic_check
> > > > step, so that any issues with the FB layout can be reported as early as
> > > > possible.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++++------
> > > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > index d9631fe90228..a9de1fbd0df3 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > @@ -673,12 +673,6 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
> > > > }
> > > > }
> > > > - ret = dpu_format_populate_plane_sizes(new_state->fb, &pstate->layout);
> > > > - if (ret) {
> > > > - DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n", ret);
> > > > - return ret;
> > > > - }
> > > > -
> > > > /* validate framebuffer layout before commit */
> > > > ret = dpu_format_populate_addrs(pstate->aspace,
> > > > new_state->fb,
> > > > @@ -864,6 +858,12 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> > > > return -E2BIG;
> > > > }
> > > > + ret = dpu_format_populate_plane_sizes(new_plane_state->fb, &pstate->layout);
> > > > + if (ret) {
> > > > + DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > >
> > > I think we need another function to do the check. It seems incorrect to
> > > populate the layout to the plane state knowing it can potentially fail.
> >
> > why? The state is interim object, which is subject to checks. In other
> > parts of the atomic_check we also fill parts of the state, perform
> > checks and then destroy it if the check fails.
> >
>
> Yes, the same thing you wrote.
>
> I felt we can perform the validation and reject it before populating it in
> the state as it seems thats doable here rather than populating it without
> knowing whether it can be discarded.
But populate function does the check. It seems like an overkill to add
another function. Also I still don't see the point. It was fine to call
this function from .prepare_fb, but it's not fine to call it from
.atomic_check?
>
> > Maybe I'm missing your point here. Could you please explain what is the
> > problem from your point of view?
> >
> > >
> > > Can we move the validation part of dpu_format_populate_plane_sizes() out to
> > > another helper dpu_format_validate_plane_sizes() and use that?
> > >
> > > And then make the remaining dpu_format_populate_plane_sizes() just a void
> > > API to fill the layout?
> >
--
With best wishes
Dmitry
