Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>
Cc: <freedreno@lists.freedesktop.org>,
	<linux-arm-msm@vger.kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	<dri-devel@lists.freedesktop.org>,
	Stephen Boyd <swboyd@chromium.org>
Subject: Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
Date: Fri, 9 Jun 2023 17:00:08 -0700	[thread overview]
Message-ID: <a144f18a-bffb-d9ed-dd2e-12977404d71e@quicinc.com> (raw)
In-Reply-To: <7d8372ff-6cb3-5bcb-ff9f-386937bff218@quicinc.com>

On 6/8/2023 12:51 PM, Abhinav Kumar wrote:
> 
> 
> On 6/7/2023 2:56 PM, Dmitry Baryshkov wrote:
>> On 08/06/2023 00:05, Abhinav Kumar wrote:
>>>
>>>
>>> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
>>>> Only several SSPP blocks support such features as YUV output or 
>>>> scaling,
>>>> thus different DRM planes have different features.  Properly utilizing
>>>> all planes requires the attention of the compositor, who should
>>>> prefer simpler planes to YUV-supporting ones. Otherwise it is very easy
>>>> to end up in a situation when all featureful planes are already
>>>> allocated for simple windows, leaving no spare plane for YUV playback.
>>>>
>>>> To solve this problem make all planes virtual. Each plane is registered
>>>> as if it supports all possible features, but then at the runtime during
>>>> the atomic_check phase the driver selects backing SSPP block for each
>>>> plane.
>>>>
>>>> Note, this does not provide support for using two different SSPP blocks
>>>> for a single plane or using two rectangles of an SSPP to drive two
>>>> planes. Each plane still gets its own SSPP and can utilize either a 
>>>> solo
>>>> rectangle or both multirect rectangles depending on the resolution.
>>>>
>>>> Note #2: By default support for virtual planes is turned off and the
>>>> driver still uses old code path with preallocated SSPP block for each
>>>> plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1'
>>>> kernel parameter.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>
>>> There will be some rebase needed to switch back to encoder based 
>>> allocation so I am not going to comment on those parts and will let 
>>> you handle that when you post v3.
>>>
>>> But my questions/comments below are for other things.
>>>
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  59 +++++--
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 120 ++++++++++----
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |   4 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 
>>>> ++++++++++++++++++----
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |  24 ++-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c    |  65 ++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h    |  24 +++
>>>>   7 files changed, 413 insertions(+), 70 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index 8ef191fd002d..cdece21b81c9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct 
>>>> drm_crtc *crtc, struct drm_crtc_stat
>>>>       return 0;
>>>>   }
>>>> +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, 
>>>> struct drm_crtc_state *crtc_state)
>>>> +{
>>>> +    struct dpu_global_state *global_state;
>>>> +    struct drm_plane *plane;
>>>> +    int rc;
>>>> +
>>>> +    global_state = dpu_kms_get_global_state(crtc_state->state);
>>>> +    if (IS_ERR(global_state))
>>>> +        return PTR_ERR(global_state);
>>>> +
>>>> +    dpu_rm_release_all_sspp(global_state, crtc);
>>>> +
>>>> +    drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
>>>> +        rc = dpu_plane_virtual_assign_resources(plane, crtc,
>>>> +                            global_state,
>>>> +                            crtc_state->state);
>>>> +        if (rc)
>>>> +            return rc;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>>>>           struct drm_atomic_state *state)
>>>>   {
>>>> @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct 
>>>> drm_crtc *crtc,
>>>>       struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>>>>       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
>>>> -    const struct drm_plane_state *pstate;
>>>>       struct drm_plane *plane;
>>>>       int rc = 0;
>>>> @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct 
>>>> drm_crtc *crtc,
>>>>               return rc;
>>>>       }
>>>> +    if (dpu_use_virtual_planes &&
>>>> +        crtc_state->planes_changed) {
>>>> +        rc = dpu_crtc_assign_plane_resources(crtc, crtc_state);
>>>> +        if (rc < 0)
>>>> +            return rc;
>>>> +    }
>>>
>>> Can you please explain a bit more about the planes_changed condition?
>>>
>>> 1) Are we doing this because the plane's atomic check happens before 
>>> the CRTC atomic check?
>>
>> Yes. Another alternative would be to stop using 
>> drm_atomic_helper_check() and implement our own code instead, 
>> inverting the plane / CRTC check order.
>>
> 
> I am fine with that too but as you noted in (3), if planes_changed will 
> be set by those functions and you can drop explicit assignment of it in 
> this patch, that will be the best option for me.
> 
>>>
>>> 2) So the DRM core sets this to true already when plane is switching 
>>> CRTCs or being connected to a CRTC for the first time, we need to 
>>> handle the conditions additional to that right?
>>
>> Nit: it is not possible to switch CRTCs. Plane first has to be 
>> disconnected and then to be connected to another CRTC.
>>
>>>
>>> 3) If (2) is correct, arent there other conditions then to be handled 
>>> for setting planes_changed to true?
>>>
>>> Some examples include, switching from a scaling to non-scaling 
>>> scenario, needing rotation vs not needing etc.
>>
>> Setting the plane_changed is not required. Both 
>> drm_atomic_helper_disable_plane() and drm_atomic_helper_update_plane() 
>> will add the plane to the state. Then drm_atomic_helper_check_planes() 
>> will call drm_atomic_helper_plane_changed(), setting 
>> {old_,new_}crtc_state->planes_changed.
>>
>> I should check if the format check below is required or not. It looks 
>> like I can drop that clause too.
>>
> 
> Yes, please do. From the above explanation, it seems like we dont need 
> to explicity set it again ourselves for format change.
> 
>>

I have a basic doubt about the split between dpu_plane_atomic_check Vs 
dpu_crtc_atomic_check() because the next patch is also relying heavily 
on the fact that plane's atomic check comes first and then crtc.

Please help me understand this better.

-> dpu_plane_virtual_atomic_check() is called and today that doesnt seem 
to do much :

	- marks visible as false if there is no fb
	(wouldnt the DRM core already do this?)
	- caches the format
	(this part is used in the dpu_crtc_atomic_check())

-> dpu_crtc_atomic_check() gets called which calls 
dpu_crtc_assign_plane_resources() which finally reserves the SSPPs to 
back the planes

-> perform dpu_plane_atomic_check() on each of the planes on the CRTC by 
checking the planes attached to CRTC state

1) What would be wrong to continue using dpu_plane_atomic_check() 
instead of the newly created dpu_plane_virtual_atomic_check() to get the 
backing SSPPs because it seems we need to maintain lot of information in 
the dpu_plane_state to manage co-ordination between the two atomic 
checks? A big portion of even the next patch does that.

2) Even dpu_plane_atomic_check() /  dpu_plane_virtual_atomic_check() is 
called only for each plane in the CRTC state, so why all the movement to 
crtc's atomic_check()? I am missing the link here. Was it done only to 
move all resource allocation to CRTC ID?