Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
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 --- 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
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
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 --- 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
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
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 --- 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. 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. Basically it seems like all of this
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
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 --- 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? 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? 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. Basically it seems like all of this is handled within the reserve_sspp() function but if planes_changes is not set then that wont get invoked at all. + if (!crtc_state->enable || !crtc_state->active) { DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n", crtc->base.id, crtc_state->enable, @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, if (cstate->num_mixers) _dpu_crtc_setup_lm_bounds(crtc, crtc_state); - /* FIXME: move this to
[RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
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 --- 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; + } + if (!crtc_state->enable || !crtc_state->active) { DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n", crtc->base.id, crtc_state->enable, @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, if (cstate->num_mixers) _dpu_crtc_setup_lm_bounds(crtc, 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); - - if (IS_ERR_OR_NULL(pstate)) { - rc = PTR_ERR(pstate); - DPU_ERROR("%s: failed to get plane%d state, %d\n", - dpu_crtc->name, plane->base.id, rc); - return rc; + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { + const struct drm_plane_state *pstate; + struct dpu_plane_state *dpu_pstate; + + pstate = drm_atomic_get_plane_state(crtc_state->state, plane); + if (IS_ERR(pstate)) + return PTR_ERR(pstate); + + if (dpu_use_virtual_planes) { + /* +* In case of virtual planes, the plane's atomic_check +* is a shortcut. Perform actual