Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 11/1/2024 2:27 PM, Abhinav Kumar wrote:
On 11/1/2024 1:53 PM, Dmitry Baryshkov wrote:
On Fri, Nov 01, 2024 at 01:37:03PM -0700, Abhinav Kumar wrote:
On 10/31/2024 2:03 PM, Dmitry Baryshkov wrote:
On Thu, Oct 31, 2024 at 01:06:34PM -0700, Abhinav Kumar wrote:
On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
Hi Abhinav,
On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar
wrote:
On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder,
the SSPP
blocks are also allocated per CRTC ID (all other resources are
currently
allocated per encoder ID). This also matches the hardware
requirement,
where both rectangles of a single SSPP can only be used with
the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool
dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state,
crtc, states, total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct
drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed ||
crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the
client.
I don't think we r
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 11/1/2024 1:53 PM, Dmitry Baryshkov wrote:
On Fri, Nov 01, 2024 at 01:37:03PM -0700, Abhinav Kumar wrote:
On 10/31/2024 2:03 PM, Dmitry Baryshkov wrote:
On Thu, Oct 31, 2024 at 01:06:34PM -0700, Abhinav Kumar wrote:
On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
Hi Abhinav,
On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar wrote:
On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder, the SSPP
blocks are also allocated per CRTC ID (all other resources are currently
allocated per encoder ID). This also matches the hardware requirement,
where both rectangles of a single SSPP can only be used with the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state
*cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state, crtc, states,
total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed || crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the client.
I don't think we really care about the modeset per se. We care about
plane size changes. And changi
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On Fri, Nov 01, 2024 at 01:37:03PM -0700, Abhinav Kumar wrote:
>
>
> On 10/31/2024 2:03 PM, Dmitry Baryshkov wrote:
> > On Thu, Oct 31, 2024 at 01:06:34PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
> > > > Hi Abhinav,
> > > >
> > > > On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar
> > > > wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
> > > > > > On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 10/24/2024 5:20 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.
> > > > > > > >
> > > > > > > > As the planes are attached to the CRTC and not the encoder, the
> > > > > > > > SSPP
> > > > > > > > blocks are also allocated per CRTC ID (all other resources are
> > > > > > > > currently
> > > > > > > > allocated per encoder ID). This also matches the hardware
> > > > > > > > requirement,
> > > > > > > > where both rectangles of a single SSPP can only be used with
> > > > > > > > the LM
> > > > > > > > pair.
> > > > > > > >
> > > > > > > > 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 | 50 +++
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
> > > > > > > > ++
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
> > > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
> > > > > > > > 7 files changed, 383 insertions(+), 29 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > > index 58595dcc3889..a7eea094aa14 100644
> > > > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > > @@ -1166,6 +1166,49 @@ static bool
> > > > > > > > dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
> > > > > > > > return false;
> > > > > > > > }
> > > > > > > > +static int dpu_crtc_reassign_planes(struct drm_crtc *crtc,
> > > > > > > > struct drm_crtc_state *crtc_state)
> > > > > > > > +{
> > > > > > > > + int total_planes = crtc->dev->mode_config.num_total_plane;
> > > > > > > > + struct drm_atomic_state *state = crtc_state->state;
> > > > > > > > + struct dpu_global_state *global_state;
> > > > > > > > + struct drm_plane_state **states;
> > > > > > > > + struct drm_plane *plane;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + 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);
> > > > > > > > +
> > > > > > > > + if (!crtc_state->enable)
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > + states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
> > > > > > > > + if
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 10/31/2024 2:03 PM, Dmitry Baryshkov wrote:
On Thu, Oct 31, 2024 at 01:06:34PM -0700, Abhinav Kumar wrote:
On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
Hi Abhinav,
On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar wrote:
On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder, the SSPP
blocks are also allocated per CRTC ID (all other resources are currently
allocated per encoder ID). This also matches the hardware requirement,
where both rectangles of a single SSPP can only be used with the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state
*cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state, crtc, states,
total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed || crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the client.
I don't think we really care about the modeset per se. We care about
plane size changes. And changing the size means that the plane is
included into the commit.
The global state mapping for SSPPs has to be cleared across mo
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On Thu, Oct 31, 2024 at 01:06:34PM -0700, Abhinav Kumar wrote:
>
>
> On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
> > Hi Abhinav,
> >
> > On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar
> > wrote:
> > >
> > >
> > >
> > > On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
> > > > On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
> > > > >
> > > > >
> > > > > On 10/24/2024 5:20 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.
> > > > > >
> > > > > > As the planes are attached to the CRTC and not the encoder, the SSPP
> > > > > > blocks are also allocated per CRTC ID (all other resources are
> > > > > > currently
> > > > > > allocated per encoder ID). This also matches the hardware
> > > > > > requirement,
> > > > > > where both rectangles of a single SSPP can only be used with the LM
> > > > > > pair.
> > > > > >
> > > > > > 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 | 50 +++
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
> > > > > > ++
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
> > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
> > > > > > 7 files changed, 383 insertions(+), 29 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > index 58595dcc3889..a7eea094aa14 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > @@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct
> > > > > > drm_crtc_state *cstate)
> > > > > > return false;
> > > > > > }
> > > > > > +static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
> > > > > > drm_crtc_state *crtc_state)
> > > > > > +{
> > > > > > + int total_planes = crtc->dev->mode_config.num_total_plane;
> > > > > > + struct drm_atomic_state *state = crtc_state->state;
> > > > > > + struct dpu_global_state *global_state;
> > > > > > + struct drm_plane_state **states;
> > > > > > + struct drm_plane *plane;
> > > > > > + int ret;
> > > > > > +
> > > > > > + 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);
> > > > > > +
> > > > > > + if (!crtc_state->enable)
> > > > > > + return 0;
> > > > > > +
> > > > > > + states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
> > > > > > + if (!states)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
> > > > > > + struct drm_plane_state *plane_state =
> > > > > > + drm_atomic_get_plane_state(state, plane);
> > > > > > +
> > > > > > + if (IS_ERR(plane_state)) {
> > > > > > + ret = PTR_ERR(plane_state);
> > > > > > + goto done;
> > > > > > + }
> > > > > > +
> > > > > > + states[plane_state->normalized_zpos] = plane_state;
> > > > > > + }
> > > > > > +
> > > > > > + ret = dpu_assig
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 10/31/2024 8:11 AM, Dmitry Baryshkov wrote:
Hi Abhinav,
On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar wrote:
On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder, the SSPP
blocks are also allocated per CRTC ID (all other resources are currently
allocated per encoder ID). This also matches the hardware requirement,
where both rectangles of a single SSPP can only be used with the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state
*cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state, crtc, states,
total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed || crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the client.
I don't think we really care about the modeset per se. We care about
plane size changes. And changing the size means that the plane is
included into the commit.
The global state mapping for SSPPs has to be cleared across modesets
IMO. This is no different from us calling dpu_rm_release() today in
dpu_encoder_virt_atomic_check(). I just am not sure w
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On Thu, 31 Oct 2024 at 17:11, Dmitry Baryshkov
wrote:
>
> Hi Abhinav,
>
> On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar wrote:
> > On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
> > > On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
> > >> On 10/24/2024 5:20 PM, Dmitry Baryshkov wrote:
> > >>> + if (reqs->yuv && !hw_sspp->cap->sblk->csc_blk.len)
> > >>> + continue;
> > >>
> > >> same here
> > >>> +
> > >>> + if (reqs->rot90 && !(hw_sspp->cap->features &
> > >>> DPU_SSPP_INLINE_ROTATION))
> > >>> + continue;
> > >>> +
> > >>> + global_state->sspp_to_crtc_id[i] = crtc_id;
> > >>> +
> > >>> + return rm->hw_sspp[i];
> > >>> + }
> > >>> +
> > >>> + return NULL;
> > >>> +}
> > >>> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm,
> > >>> + struct dpu_global_state
> > >>> *global_state,
> > >>> + struct drm_crtc *crtc,
> > >>> + struct dpu_rm_sspp_requirements
> > >>> *reqs)
> > >>> +{
> > >>> + struct dpu_hw_sspp *hw_sspp = NULL;
> > >>> +
> > >>> + if (!reqs->scale && !reqs->yuv)
> > >>> + hw_sspp = dpu_rm_try_sspp(rm, global_state, crtc, reqs,
> > >>> SSPP_TYPE_DMA);
> > >>> + if (!hw_sspp && reqs->scale)
> > >>> + hw_sspp = dpu_rm_try_sspp(rm, global_state, crtc, reqs,
> > >>> SSPP_TYPE_RGB);
> > >>
> > >> I dont recollect whether RGB SSPPs supported scaling, if you have any
> > >> source
> > >> or link for this, that would help me for sure.
> > >
> > > I have to dig further into the old fbdev driver. It looks like
> > > mdss_mdp_qseed2_setup() is getting called for all plane types on the
> > > corresponding hardware, but then it rejects scaling only for DMA and
> > > CURSOR planes, which means that RGB planes should get the scaler setup.
> > >
> > > For now this is from the SDE driver from 4.4:
> > >
> > > * @SDE_SSPP_SCALER_RGB, RGB Scaler, supported by RGB pipes
> > >
> > >> But even otherwise, I dont see any chipset in the catalog setting this
> > >> SSPP
> > >> type, so do we need to add this case?
> > >
> > > Yes, we do. MSM8996 / MSM8937 / MSM8917 / MSM8953 use RGB planes.
> > >
> >
> > Yes those chipsets do have RGB SSPP. My question was whether they have
> > migrated to dpu and thats why I wanted to know whether we want to
> > include RGB SSPP handling.
> >
> > I do not even see them in msm_mdp5_dpu_migration.
>
> Ugh, it's a bug then, I'll push a fix.
Ok, this is very surprising:
static const char *const msm_mdp5_dpu_migration[] = {
"qcom,msm8917-mdp5",
"qcom,msm8937-mdp5",
"qcom,msm8953-mdp5",
"qcom,msm8996-mdp5",
"qcom,sdm630-mdp5",
"qcom,sdm660-mdp5",
NULL,
};
--
With best wishes
Dmitry
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
Hi Abhinav,
On Wed, 30 Oct 2024 at 21:26, Abhinav Kumar wrote:
>
>
>
> On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
> > On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
> >>
> >>
> >> On 10/24/2024 5:20 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.
> >>>
> >>> As the planes are attached to the CRTC and not the encoder, the SSPP
> >>> blocks are also allocated per CRTC ID (all other resources are currently
> >>> allocated per encoder ID). This also matches the hardware requirement,
> >>> where both rectangles of a single SSPP can only be used with the LM
> >>> pair.
> >>>
> >>> 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 | 50 +++
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
> >>> ++
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
> >>>drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
> >>>7 files changed, 383 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> index 58595dcc3889..a7eea094aa14 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> @@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct
> >>> drm_crtc_state *cstate)
> >>> return false;
> >>>}
> >>> +static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
> >>> drm_crtc_state *crtc_state)
> >>> +{
> >>> + int total_planes = crtc->dev->mode_config.num_total_plane;
> >>> + struct drm_atomic_state *state = crtc_state->state;
> >>> + struct dpu_global_state *global_state;
> >>> + struct drm_plane_state **states;
> >>> + struct drm_plane *plane;
> >>> + int ret;
> >>> +
> >>> + 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);
> >>> +
> >>> + if (!crtc_state->enable)
> >>> + return 0;
> >>> +
> >>> + states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
> >>> + if (!states)
> >>> + return -ENOMEM;
> >>> +
> >>> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
> >>> + struct drm_plane_state *plane_state =
> >>> + drm_atomic_get_plane_state(state, plane);
> >>> +
> >>> + if (IS_ERR(plane_state)) {
> >>> + ret = PTR_ERR(plane_state);
> >>> + goto done;
> >>> + }
> >>> +
> >>> + states[plane_state->normalized_zpos] = plane_state;
> >>> + }
> >>> +
> >>> + ret = dpu_assign_plane_resources(global_state, state, crtc, states,
> >>> total_planes);
> >>> +
> >>> +done:
> >>> + kfree(states);
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>>static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> >>> struct drm_atomic_state *state)
> >>>{
> >>> @@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> >>> *crtc,
> >>> bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
> >>> + if (dpu_use_virtual_planes &&
> >>> + (crtc_state->planes_changed || crtc_state->zpos_changed)) {
> >>> + rc = dpu_crtc_reassign_planes(crtc, crtc_state);
> >>> + if (rc < 0)
> >>> + return rc;
> >>> + }
> >>
> >> planes_changed is set only for format changes . Will it cover all
> >> needs_modeset cases?
> >>
> >> OR do we als
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 10/30/2024 3:48 AM, Dmitry Baryshkov wrote:
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder, the SSPP
blocks are also allocated per CRTC ID (all other resources are currently
allocated per encoder ID). This also matches the hardware requirement,
where both rectangles of a single SSPP can only be used with the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state
*cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state, crtc, states,
total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed || crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the client.
I don't think we really care about the modeset per se. We care about
plane size changes. And changing the size means that the plane is
included into the commit.
The global state mapping for SSPPs has to be cleared across modesets
IMO. This is no different from us calling dpu_rm_release() today in
dpu_encoder_virt_atomic_check(). I
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On Tue, Oct 29, 2024 at 02:30:12PM -0700, Abhinav Kumar wrote:
>
>
> On 10/24/2024 5:20 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.
> >
> > As the planes are attached to the CRTC and not the encoder, the SSPP
> > blocks are also allocated per CRTC ID (all other resources are currently
> > allocated per encoder ID). This also matches the hardware requirement,
> > where both rectangles of a single SSPP can only be used with the LM
> > pair.
> >
> > 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 | 50 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237
> > ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
> > 7 files changed, 383 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 58595dcc3889..a7eea094aa14 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct
> > drm_crtc_state *cstate)
> > return false;
> > }
> > +static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct
> > drm_crtc_state *crtc_state)
> > +{
> > + int total_planes = crtc->dev->mode_config.num_total_plane;
> > + struct drm_atomic_state *state = crtc_state->state;
> > + struct dpu_global_state *global_state;
> > + struct drm_plane_state **states;
> > + struct drm_plane *plane;
> > + int ret;
> > +
> > + 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);
> > +
> > + if (!crtc_state->enable)
> > + return 0;
> > +
> > + states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
> > + if (!states)
> > + return -ENOMEM;
> > +
> > + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
> > + struct drm_plane_state *plane_state =
> > + drm_atomic_get_plane_state(state, plane);
> > +
> > + if (IS_ERR(plane_state)) {
> > + ret = PTR_ERR(plane_state);
> > + goto done;
> > + }
> > +
> > + states[plane_state->normalized_zpos] = plane_state;
> > + }
> > +
> > + ret = dpu_assign_plane_resources(global_state, state, crtc, states,
> > total_planes);
> > +
> > +done:
> > + kfree(states);
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> > struct drm_atomic_state *state)
> > {
> > @@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
> > + if (dpu_use_virtual_planes &&
> > + (crtc_state->planes_changed || crtc_state->zpos_changed)) {
> > + rc = dpu_crtc_reassign_planes(crtc, crtc_state);
> > + if (rc < 0)
> > + return rc;
> > + }
>
> planes_changed is set only for format changes . Will it cover all
> needs_modeset cases?
>
> OR do we also need to set planes_changed when
> drm_atomic_crtc_needs_modeset()?
>
> Unless I am missing something, I think we have to otherwise sspp
> reallocation wont happen in modeset cases.
I was depending on the planes being included in the state by the client.
I don't think we really care about the modeset per se. We care about
plane size changes. And changing t
Re: [PATCH v6 7/9] drm/msm/dpu: add support for virtual planes
On 10/24/2024 5:20 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.
As the planes are attached to the CRTC and not the encoder, the SSPP
blocks are also allocated per CRTC ID (all other resources are currently
allocated per encoder ID). This also matches the hardware requirement,
where both rectangles of a single SSPP can only be used with the LM
pair.
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 | 50 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 237 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 16 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 68 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 27
7 files changed, 383 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 58595dcc3889..a7eea094aa14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1166,6 +1166,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state
*cstate)
return false;
}
+static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state)
+{
+ int total_planes = crtc->dev->mode_config.num_total_plane;
+ struct drm_atomic_state *state = crtc_state->state;
+ struct dpu_global_state *global_state;
+ struct drm_plane_state **states;
+ struct drm_plane *plane;
+ int ret;
+
+ 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);
+
+ if (!crtc_state->enable)
+ return 0;
+
+ states = kcalloc(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ drm_atomic_crtc_state_for_each_plane(plane, crtc_state) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+
+ states[plane_state->normalized_zpos] = plane_state;
+ }
+
+ ret = dpu_assign_plane_resources(global_state, state, crtc, states,
total_planes);
+
+done:
+ kfree(states);
+ return ret;
+
+ return 0;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -1181,6 +1224,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+ if (dpu_use_virtual_planes &&
+ (crtc_state->planes_changed || crtc_state->zpos_changed)) {
+ rc = dpu_crtc_reassign_planes(crtc, crtc_state);
+ if (rc < 0)
+ return rc;
+ }
planes_changed is set only for format changes . Will it cover all
needs_modeset cases?
OR do we also need to set planes_changed when
drm_atomic_crtc_needs_modeset()?
Unless I am missing something, I think we have to otherwise sspp
reallocation wont happen in modeset cases.
Overall, mainly we want to make sure SSPPs are re-assigned when:
1) format changes (RGB to YUV and vice-versa)
2) Any modesets
3) Any disable/enable without modeset like connectors changed as SSPPs
are changing outputs there.
If we are covered for all these, let me know.
+
if (!crtc_state->enable ||
!drm_atomic_crtc_effectively_active(crtc_state)) {
DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip
atomic_check\n",
crtc->base.id, crtc_state->enable,
diff --gi
