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

[Abhinav Kumar]

On 4/22/2024 5:22 AM, Dmitry Baryshkov wrote:
> 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?
> 

As we discussed during the meeting, discarding rejected state is fine 
and is a commonly used practice, hence this is okay. I was only trying 
to avoid that.

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