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