The ingenic-drm driver has two mutually exclusive primary planes
already; so the fact that a CRTC must have one and only one primary
plane is an invalid assumption.
Fixes: 96962e3de725 ("drm: require each CRTC to have a unique primary plane")
Cc: <[email protected]> # 5.11
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/drm_mode_config.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 37b4b9f0e468..d86621c41047 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -626,9 +626,7 @@ void drm_mode_config_validate(struct drm_device *dev)
{
struct drm_encoder *encoder;
struct drm_crtc *crtc;
- struct drm_plane *plane;
u32 primary_with_crtc = 0, cursor_with_crtc = 0;
- unsigned int num_primary = 0;
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return;
@@ -676,13 +674,4 @@ void drm_mode_config_validate(struct drm_device *dev)
cursor_with_crtc |= drm_plane_mask(crtc->cursor);
}
}
-
- drm_for_each_plane(plane, dev) {
- if (plane->type == DRM_PLANE_TYPE_PRIMARY)
- num_primary++;
- }
-
- WARN(num_primary != dev->mode_config.num_crtc,
- "Must have as many primary planes as there are CRTCs, but have %u primary planes and %u CRTCs",
- num_primary, dev->mode_config.num_crtc);
}
--
2.30.2
On Sat, Mar 27, 2021 at 11:22:14AM +0000, Paul Cercueil wrote:
> The ingenic-drm driver has two mutually exclusive primary planes
> already; so the fact that a CRTC must have one and only one primary
> plane is an invalid assumption.
I mean, no? It's been documented for a while that a CRTC should only
have a single primary, so I'd say that the invalid assumption was that
it was possible to have multiple primary planes for a CRTC.
Since it looks like you have two mutually exclusive planes, just expose
one and be done with it?
Maxime
On Monday, March 29th, 2021 at 4:07 PM, Maxime Ripard <[email protected]> wrote:
> Since it looks like you have two mutually exclusive planes, just expose
> one and be done with it?
You can expose the other as an overlay. Clever user-space will be able
to figure out that the more advanced plane can be used if the primary
plane is disabled.
But yeah, I don't think exposing two primary planes makes sense. The
"primary" bit is just there for legacy user-space, it's a hint that
it's the best plane to light up for fullscreen content. It has no other
significance than that, and in particular it doesn't mean that it's
incompatible with other primary planes.
Hi Maxime,
Le lun. 29 mars 2021 ? 16:07, Maxime Ripard <[email protected]> a
?crit :
> On Sat, Mar 27, 2021 at 11:22:14AM +0000, Paul Cercueil wrote:
>> The ingenic-drm driver has two mutually exclusive primary planes
>> already; so the fact that a CRTC must have one and only one primary
>> plane is an invalid assumption.
>
> I mean, no? It's been documented for a while that a CRTC should only
> have a single primary, so I'd say that the invalid assumption was that
> it was possible to have multiple primary planes for a CRTC.
Documented where?
I did read the doc of "enum drm_plane_type" in <drm/drm_plane.h>, and
the DRM_PLANE_TYPE_PRIMARY describes my two planes, so I went with that.
-Paul
> Since it looks like you have two mutually exclusive planes, just
> expose
> one and be done with it?
>
> Maxime
Hi Simon,
Le lun. 29 mars 2021 ? 14:11, Simon Ser <[email protected]> a ?crit
:
> On Monday, March 29th, 2021 at 4:07 PM, Maxime Ripard
> <[email protected]> wrote:
>
>> Since it looks like you have two mutually exclusive planes, just
>> expose
>> one and be done with it?
>
> You can expose the other as an overlay. Clever user-space will be able
> to figure out that the more advanced plane can be used if the primary
> plane is disabled.
>
> But yeah, I don't think exposing two primary planes makes sense. The
> "primary" bit is just there for legacy user-space, it's a hint that
> it's the best plane to light up for fullscreen content. It has no
> other
> significance than that, and in particular it doesn't mean that it's
> incompatible with other primary planes.
Yes, from what I understood when writing the driver, there is not much
of a difference with primary vs. overlay planes when dealing with the
atomic DRM API, which I used exclusively.
Making the second plane an overlay would break the ABI, which is never
something I'm happy to do; but I'd prefer to do it now than later.
I still have concerns about the user-space being "clever" enough to
know it can disable the primary plane. Can e.g. wlroots handle that?
Cheers,
-Paul
On Mon, Mar 29, 2021 at 04:15:28PM +0100, Paul Cercueil wrote:
> Hi Maxime,
>
> Le lun. 29 mars 2021 ? 16:07, Maxime Ripard <[email protected]> a ?crit :
> > On Sat, Mar 27, 2021 at 11:22:14AM +0000, Paul Cercueil wrote:
> > > The ingenic-drm driver has two mutually exclusive primary planes
> > > already; so the fact that a CRTC must have one and only one primary
> > > plane is an invalid assumption.
> >
> > I mean, no? It's been documented for a while that a CRTC should only
> > have a single primary, so I'd say that the invalid assumption was that
> > it was possible to have multiple primary planes for a CRTC.
>
> Documented where?
>
> I did read the doc of "enum drm_plane_type" in <drm/drm_plane.h>, and the
> DRM_PLANE_TYPE_PRIMARY describes my two planes, so I went with that.
At least since 4.9, this was in the documentation generated for DRM:
https://elixir.bootlin.com/linux/v4.9.263/source/drivers/gpu/drm/drm_plane.c#L43
Maxime
On Monday, March 29th, 2021 at 5:32 PM, Paul Cercueil <[email protected]> wrote:
> Making the second plane an overlay would break the ABI, which is never
> something I'm happy to do; but I'd prefer to do it now than later.
Yeah, I wonder if some user-space depends on this behavior somehow?
> I still have concerns about the user-space being "clever" enough to
> know it can disable the primary plane. Can e.g. wlroots handle that?
wlroots will always pick the first primary plane, and will never use
overlays. The plan is to use libliftoff [1] to make use of overlay
planes. libliftoff should already support the scenario you describe.
I think Weston supports that too.
[1]: https://github.com/emersion/libliftoff
Le lun. 29 mars 2021 ? 17:35, Maxime Ripard <[email protected]> a
?crit :
> On Mon, Mar 29, 2021 at 04:15:28PM +0100, Paul Cercueil wrote:
>> Hi Maxime,
>>
>> Le lun. 29 mars 2021 ? 16:07, Maxime Ripard <[email protected]> a
>> ?crit :
>> > On Sat, Mar 27, 2021 at 11:22:14AM +0000, Paul Cercueil wrote:
>> > > The ingenic-drm driver has two mutually exclusive primary
>> planes
>> > > already; so the fact that a CRTC must have one and only one
>> primary
>> > > plane is an invalid assumption.
>> >
>> > I mean, no? It's been documented for a while that a CRTC should
>> only
>> > have a single primary, so I'd say that the invalid assumption was
>> that
>> > it was possible to have multiple primary planes for a CRTC.
>>
>> Documented where?
>>
>> I did read the doc of "enum drm_plane_type" in <drm/drm_plane.h>,
>> and the
>> DRM_PLANE_TYPE_PRIMARY describes my two planes, so I went with that.
>
> At least since 4.9, this was in the documentation generated for DRM:
> https://elixir.bootlin.com/linux/v4.9.263/source/drivers/gpu/drm/drm_plane.c#L43
Ok, I read that as "all drivers should provide AT LEAST one primary
plane per CRTC", and not as "all drivers should provide ONE AND ONLY
ONE primary plane per CRTC". My bad.
-Paul
On Monday, March 29th, 2021 at 5:39 PM, Paul Cercueil <[email protected]> wrote:
> Ok, I read that as "all drivers should provide AT LEAST one primary
> plane per CRTC", and not as "all drivers should provide ONE AND ONLY
> ONE primary plane per CRTC". My bad.
Yeah, it's a little complicated to document, because it's possible for
a primary plane to be compatible with multiple CRTCs… We tried to
improve this [1] recently.
[1]: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-abstraction
Le lun. 29 mars 2021 à 15:42, Simon Ser <[email protected]> a écrit
:
> On Monday, March 29th, 2021 at 5:39 PM, Paul Cercueil
> <[email protected]> wrote:
>
>> Ok, I read that as "all drivers should provide AT LEAST one primary
>> plane per CRTC", and not as "all drivers should provide ONE AND ONLY
>> ONE primary plane per CRTC". My bad.
>
> Yeah, it's a little complicated to document, because it's possible for
> a primary plane to be compatible with multiple CRTCs… We tried to
> improve this [1] recently.
>
> [1]:
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-abstraction
Ok, that is definitely much clearer :)
-Paul
On Mon, 29 Mar 2021 15:36:27 +0000
Simon Ser <[email protected]> wrote:
> On Monday, March 29th, 2021 at 5:32 PM, Paul Cercueil <[email protected]> wrote:
>
> > Making the second plane an overlay would break the ABI, which is never
> > something I'm happy to do; but I'd prefer to do it now than later.
>
> Yeah, I wonder if some user-space depends on this behavior somehow?
>
> > I still have concerns about the user-space being "clever" enough to
> > know it can disable the primary plane. Can e.g. wlroots handle that?
>
> wlroots will always pick the first primary plane, and will never use
> overlays. The plan is to use libliftoff [1] to make use of overlay
> planes. libliftoff should already support the scenario you describe.
>
> I think Weston supports that too.
Weston supports overlays, but I don't think it will try without "the"
primary plane, IIRC. I'd need to verify.
I'm not quite sure what Weston would do with multiple primary planes.
It probably picks one for a CRTC ahead of time, and then sticks to it,
always using it.
But if Weston never worked with a driver to begin with, it also can't
regress, so you're safe.
Thanks,
pq
>
> [1]: https://github.com/emersion/libliftoff
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel