diff mbox series

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

Message ID 20240319-dpu-mode-config-width-v1-4-d0fe6bf81bf1@linaro.org
State Superseded
Headers show
Series [1/9] drm/msm/dpu: drop dpu_format_check_modified_format | expand

Commit Message

Dmitry Baryshkov March 19, 2024, 1:22 p.m. UTC
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@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Abhinav Kumar April 20, 2024, 12:14 a.m. UTC | #1
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@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.

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?
Dmitry Baryshkov April 20, 2024, 1:34 a.m. UTC | #2
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@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.

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?
Abhinav Kumar April 20, 2024, 2:37 a.m. UTC | #3
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@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.

> 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?
>
diff mbox series

Patch

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;
+	}
+
 	fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
 
 	max_linewidth = pdpu->catalog->caps->max_linewidth;