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: stable@vger.kernel.org # 5.11 Signed-off-by: Paul Cercueil paul@crapouillou.net --- 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); }
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only has a single CRTC?
It has two mutually exclusive background planes (same Z level) + one overlay plane.
-Paul
Le sam. 27 mars 2021 à 11:24, Simon Ser contact@emersion.fr a écrit :
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only has a single CRTC?
On Sat, 27 Mar 2021 11:26:26 +0000 Paul Cercueil paul@crapouillou.net wrote:
It has two mutually exclusive background planes (same Z level) + one overlay plane.
What's the difference between the two background planes?
How will generic userspace know to pick the "right" one?
Thanks, pq
Le sam. 27 mars 2021 à 11:24, Simon Ser contact@emersion.fr a écrit :
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only has a single CRTC?
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
Le lun. 29 mars 2021 à 11:15, Pekka Paalanen ppaalanen@gmail.com a écrit :
On Sat, 27 Mar 2021 11:26:26 +0000 Paul Cercueil paul@crapouillou.net wrote:
It has two mutually exclusive background planes (same Z level) + one overlay plane.
What's the difference between the two background planes?
How will generic userspace know to pick the "right" one?
First primary plane cannot scale, supports RGB and C8. Second primary plane goes through the IPU, and as such can scale and convert pixel formats; it supports RGB, non-planar YUV, and multi-planar YUV.
Right now the userspace apps we have will simply pick the first one that fits the bill.
Cheers, -Paul
Le sam. 27 mars 2021 à 11:24, Simon Ser contact@emersion.fr a écrit :
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only has a single CRTC?
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 29 Mar 2021 12:41:00 +0100 Paul Cercueil paul@crapouillou.net wrote:
Hi,
Le lun. 29 mars 2021 à 11:15, Pekka Paalanen ppaalanen@gmail.com a écrit :
On Sat, 27 Mar 2021 11:26:26 +0000 Paul Cercueil paul@crapouillou.net wrote:
It has two mutually exclusive background planes (same Z level) + one overlay plane.
What's the difference between the two background planes?
How will generic userspace know to pick the "right" one?
First primary plane cannot scale, supports RGB and C8. Second primary plane goes through the IPU, and as such can scale and convert pixel formats; it supports RGB, non-planar YUV, and multi-planar YUV.
Right now the userspace apps we have will simply pick the first one that fits the bill.
What would be the downside of exposing just one "virtual" primary plane, and then have the driver pick one of the two hardware planes as appropriate per modeset?
Thanks, pq
Le sam. 27 mars 2021 à 11:24, Simon Ser contact@emersion.fr a écrit :
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only has a single CRTC?
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Le lun. 29 mars 2021 à 17:35, Pekka Paalanen ppaalanen@gmail.com a écrit :
On Mon, 29 Mar 2021 12:41:00 +0100 Paul Cercueil paul@crapouillou.net wrote:
Hi,
Le lun. 29 mars 2021 à 11:15, Pekka Paalanen ppaalanen@gmail.com a écrit :
On Sat, 27 Mar 2021 11:26:26 +0000 Paul Cercueil paul@crapouillou.net wrote:
It has two mutually exclusive background planes (same Z level)
- one
overlay plane.
What's the difference between the two background planes?
How will generic userspace know to pick the "right" one?
First primary plane cannot scale, supports RGB and C8. Second primary plane goes through the IPU, and as such can scale and convert pixel formats; it supports RGB, non-planar YUV, and multi-planar YUV.
Right now the userspace apps we have will simply pick the first one that fits the bill.
What would be the downside of exposing just one "virtual" primary plane, and then have the driver pick one of the two hardware planes as appropriate per modeset?
The IPU plane is in a different driver, so all the callbacks are different. That sounds like it would be a mess.
-Paul
Thanks, pq
Le sam. 27 mars 2021 à 11:24, Simon Ser contact@emersion.fr a écrit :
On Saturday, March 27th, 2021 at 12:22 PM, Paul Cercueil paul@crapouillou.net 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.
Why does this driver expose two primary planes, if it only
has a
single CRTC?
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 maxime@cerno.tech 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 Simon,
Le lun. 29 mars 2021 à 14:11, Simon Ser contact@emersion.fr a écrit :
On Monday, March 29th, 2021 at 4:07 PM, Maxime Ripard maxime@cerno.tech 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 Monday, March 29th, 2021 at 5:32 PM, Paul Cercueil paul@crapouillou.net 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.
On Mon, 29 Mar 2021 15:36:27 +0000 Simon Ser contact@emersion.fr wrote:
On Monday, March 29th, 2021 at 5:32 PM, Paul Cercueil paul@crapouillou.net 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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Maxime,
Le lun. 29 mars 2021 à 16:07, Maxime Ripard maxime@cerno.tech 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
On Mon, Mar 29, 2021 at 04:15:28PM +0100, Paul Cercueil wrote:
Hi Maxime,
Le lun. 29 mars 2021 à 16:07, Maxime Ripard maxime@cerno.tech 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...
Maxime
Le lun. 29 mars 2021 à 17:35, Maxime Ripard maxime@cerno.tech 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 maxime@cerno.tech 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...
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 paul@crapouillou.net 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 contact@emersion.fr a écrit :
On Monday, March 29th, 2021 at 5:39 PM, Paul Cercueil paul@crapouillou.net 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.
Ok, that is definitely much clearer :)
-Paul
linux-stable-mirror@lists.linaro.org
-
Maxime Ripard -
Paul Cercueil -
Pekka Paalanen -
Simon Ser