[PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check

[Dmitry Baryshkov]

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 at linaro.org>
> > > > ---
> > > >    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