| Message ID | 20210327112214.10252-1-paul@crapouillou.net |
|---|---|
| State | New |
| Headers | show |
| Series | drm: DON'T require each CRTC to have a unique primary plane | expand |
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 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.
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 > > > >
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
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 >> > >> >> >
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 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#L43 Maxime
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. [1]: https://github.com/emersion/libliftoff
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#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 <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. > > [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 <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 > > [1]: https://github.com/emersion/libliftoff > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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); }
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(-)