Re: [PATCH v3 15/27] drm/msm/dpu: move the rest of plane checks to dpu_plane_atomic_check()

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 15/02/2023 02:08, Dmitry Baryshkov wrote:
> On 15/02/2023 01:25, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> Sorry for the late response on this one.
>>
>> On 2/3/2023 2:55 PM, Dmitry Baryshkov wrote:
>>> On 04/02/2023 00:44, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>> Move plane state updates from dpu_crtc_atomic_check() to the function
>>>>> where they belong: to dpu_plane_atomic_check().
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 18 +-----------------
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 ++++++++++--------
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |  6 ------
>>>>>   3 files changed, 11 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index b485234eefb2..bd09bb319a58 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> @@ -1129,7 +1129,6 @@ static int dpu_crtc_atomic_check(struct 
>>>>> drm_crtc *crtc,
>>>>>                                         crtc);
>>>>>       struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>>>>>       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
>>>>> -    struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>>>>>       const struct drm_plane_state *pstate;
>>>>>       struct drm_plane *plane;
>>>>> @@ -1161,11 +1160,10 @@ static int dpu_crtc_atomic_check(struct 
>>>>> drm_crtc *crtc,
>>>>>       crtc_rect.x2 = mode->hdisplay;
>>>>>       crtc_rect.y2 = mode->vdisplay;
>>>>> -     /* get plane state for all drm planes associated with crtc 
>>>>> state */
>>>>> +    /* FIXME: move this to dpu_plane_atomic_check? */
>>>>>       drm_atomic_crtc_state_for_each_plane_state(plane, pstate, 
>>>>> crtc_state) {
>>>>>           struct dpu_plane_state *dpu_pstate = 
>>>>> to_dpu_plane_state(pstate);
>>>>>           struct drm_rect dst, clip = crtc_rect;
>>>>> -        int stage;
>>>>>           if (IS_ERR_OR_NULL(pstate)) {
>>>>>               rc = PTR_ERR(pstate);
>>>>> @@ -1179,8 +1177,6 @@ static int dpu_crtc_atomic_check(struct 
>>>>> drm_crtc *crtc,
>>>>>           dpu_pstate->needs_dirtyfb = needs_dirtyfb;
>>>>> -        dpu_plane_clear_multirect(pstate);
>>>>> -
>>>>>           dst = drm_plane_state_dest(pstate);
>>>>>           if (!drm_rect_intersect(&clip, &dst)) {
>>>>>               DPU_ERROR("invalid vertical/horizontal destination\n");
>>>>> @@ -1189,18 +1185,6 @@ static int dpu_crtc_atomic_check(struct 
>>>>> drm_crtc *crtc,
>>>>>                     DRM_RECT_ARG(&dst));
>>>>>               return -E2BIG;
>>>>>           }
>>>>> -
>>>>> -        /* verify stage setting before using it */
>>>>> -        stage = DPU_STAGE_0 + pstate->normalized_zpos;
>>>>> -        if (stage >= dpu_kms->catalog->caps->max_mixer_blendstages) {
>>>>> -            DPU_ERROR("> %d plane stages assigned\n",
>>>>> -                    dpu_kms->catalog->caps->max_mixer_blendstages 
>>>>> - DPU_STAGE_0);
>>>>> -            return -EINVAL;
>>>>> -        }
>>>>> -
>>>>> -        to_dpu_plane_state(pstate)->stage = stage;
>>>>> -        DRM_DEBUG_ATOMIC("%s: stage %d\n", dpu_crtc->name, stage);
>>>>> -
>>>>>       }
>>>>>       atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref);
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> index 1b3033b15bfa..5aabf9694a53 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> @@ -733,14 +733,6 @@ static int _dpu_plane_color_fill(struct 
>>>>> dpu_plane *pdpu,
>>>>>       return 0;
>>>>>   }
>>>>> -void dpu_plane_clear_multirect(const struct drm_plane_state 
>>>>> *drm_state)
>>>>> -{
>>>>> -    struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>>>>> -
>>>>> -    pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> -    pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> -}
>>>>> -
>>>>>   int dpu_plane_validate_multirect_v2(struct 
>>>>> dpu_multirect_plane_states *plane)
>>>>>   {
>>>>>       struct dpu_plane_state *pstate[R_MAX];
>>>>> @@ -994,6 +986,16 @@ static int dpu_plane_atomic_check(struct 
>>>>> drm_plane *plane,
>>>>>       if (!new_plane_state->visible)
>>>>>           return 0;
>>>>> +    pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> +    pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +
>>>>
>>>> But I am not sure if clearing the multirect belongs here and now I 
>>>> want to clarify one thing about 
>>>> https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 
>>>> which was R-bed in the v1 and carried fwd since then.
>>>>
>>>> So prior to that change, we were only clearing the multirects of the 
>>>> planes that were staged to the crtc and we were getting those from 
>>>> the crtc state. But now we are clearing the multirect of all the 
>>>> planes.
>>>>
>>>> Dont we have to keep that in the crtc_atomic_check() since we do 
>>>> that on all the planes attached to a certain CRTC.
>>>>
>>>> In that case shouldnt we keep this in the crtc_atomic_check() and 
>>>> bring back pipe_staged[] without the multirect and source split 
>>>> cases ofcourse.
>>>
>>> What for? In other words, what would be the difference?
>>>
>>
>> So, please correct my understanding here. drm_plane's atomic_check() 
>> will be called for all the planes which are getting updated in this 
>> atomic commit using for_each_oldnew_plane_in_state() and drm_crtc's 
>> atomic_check() will be called for all the CRTC's in this atomic update 
>> using for_each_new_crtc_in_state() >
>> If the plane is not connected to any CRTC, why do we need to clear the 
>> multirect pstates.
> 
> If the plane is not connected to any CRTC, then we just don't care what 
> is there in the multirect state, so we might clear it as well.
> 
>>
>> OR in that case would atomic_commit not even be called if the plane is 
>> not connected to any CRTC?
>>
>> One case i can think of is the disable commit where the no planes will 
>> be connected to the CRTC so in that case, before this change we would 
>> explicitly clear out all the planes connected to the CRTC but now with 
>> this change is there a possibility that only if the plane state 
>> changed we would clear it out?
> 
> Ah. Maybe I understand your point. I think 
> drm_atomic_add_affected_planes() will ensure that all planes attached to 
> CRTCs are also a part of the atomic state.

Checked, it works as expected. But on the other hand, this pointed me to 
a possible issue in dpu_plane_atomic_disable. Probably we should drop 
the multirect setup there.

> 
> Regarding the change itself. Think about encapsulation. CRTC should not 
> care about plane's multirect state. It is a plane implementation detail. 
> As we delve upon a path of using rect1 and then even using different 
> SSPPs for the plane, these implementation details will change (mostly) 
> behind CRTC's back.
> 
>>
>>
>>>>
>>>>> +    pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>>>>> +    if (pstate->stage >= 
>>>>> pdpu->catalog->caps->max_mixer_blendstages) {
>>>>> +        DPU_ERROR("> %d plane stages assigned\n",
>>>>> +                pdpu->catalog->caps->max_mixer_blendstages - 
>>>>> DPU_STAGE_0);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>
>>>> I agree that this check belongs to the plane_atomic_check().
>>>>
>>>>>       src.x1 = new_plane_state->src_x >> 16;
>>>>>       src.y1 = new_plane_state->src_y >> 16;
>>>>>       src.x2 = src.x1 + (new_plane_state->src_w >> 16);
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> index 228db401e905..a08b0539513b 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> @@ -88,12 +88,6 @@ struct drm_plane *dpu_plane_init(struct 
>>>>> drm_device *dev,
>>>>>    */
>>>>>   int dpu_plane_validate_multirect_v2(struct 
>>>>> dpu_multirect_plane_states *plane);
>>>>> -/**
>>>>> - * dpu_plane_clear_multirect - clear multirect bits for the given 
>>>>> pipe
>>>>> - * @drm_state: Pointer to DRM plane state
>>>>> - */
>>>>> -void dpu_plane_clear_multirect(const struct drm_plane_state 
>>>>> *drm_state);
>>>>> -
>>>>>   /**
>>>>>    * dpu_plane_color_fill - enables color fill on plane
>>>>>    * @plane:  Pointer to DRM plane object
>>>
> 

-- 
With best wishes
Dmitry