[RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
Abhinav Kumar
quic_abhinavk at quicinc.com
Sat Jun 10 00:00:08 UTC 2023
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 at 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?
More information about the dri-devel
mailing list