On Wed, Dec 09, 2020 at 04:35:31PM +0000, Simon Ser wrote: > On Wednesday, December 9th, 2020 at 5:02 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Wed, 9 Dec 2020 01:42:23 +0100 > > Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > On Sun, Dec 06, 2020 at 04:34:15PM +0000, Simon Ser wrote: > > > > The previous wording could be understood by user-space evelopers as "a > > > > primary/cursor plane is only compatible with a single CRTC" [1]. > > > > > > > > Reword the planes description to make it clear the DRM-internal > > > > drm_crtc.primary and drm_crtc.cursor planes are for legacy uAPI. > > > > > > > > [1]: https://github.com/swaywm/wlroots/pull/2333#discussion_r456788057 > > > > > > > > Signed-off-by: Simon Ser <contact@xxxxxxxxxxx> > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx> > > > > --- > > > > drivers/gpu/drm/drm_crtc.c | 3 +++ > > > > drivers/gpu/drm/drm_plane.c | 16 +++++++++------- > > > > 2 files changed, 12 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > > index 74090fc3aa55..c71b134d663a 100644 > > > > --- a/drivers/gpu/drm/drm_crtc.c > > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > @@ -256,6 +256,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc) > > > > * planes). For really simple hardware which has only 1 plane look at > > > > * drm_simple_display_pipe_init() instead. > > > > * > > > > + * The @primary and @cursor planes are only relevant for legacy uAPI, see > > > > + * &drm_crtc.primary and &drm_crtc.cursor. > > > > + * > > > > * Returns: > > > > * Zero on success, error code on failure. > > > > */ > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > > > index e6231947f987..7a5697bc9e04 100644 > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > @@ -49,14 +49,16 @@ > > > > * &struct drm_plane (possibly as part of a larger structure) and registers it > > > > * with a call to drm_universal_plane_init(). > > > > * > > > > - * Cursor and overlay planes are optional. All drivers should provide one > > > > - * primary plane per CRTC to avoid surprising userspace too much. See enum > > > > - * drm_plane_type for a more in-depth discussion of these special uapi-relevant > > > > - * plane types. Special planes are associated with their CRTC by calling > > > > - * drm_crtc_init_with_planes(). > > > > - * > > > > * The type of a plane is exposed in the immutable "type" enumeration property, > > > > - * which has one of the following values: "Overlay", "Primary", "Cursor". > > > > + * which has one of the following values: "Overlay", "Primary", "Cursor" (see > > > > + * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > > > > + * &drm_plane.possible_crtcs. > > > > + * > > > > + * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core > > > > + * relies on the driver to set the primary and optionally the cursor plane used > > > > + * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All > > > > + * drivers should provide one primary plane per CRTC to avoid surprising legacy > > > > s/should/must/? > > Unsure about this. Do we really want to WARN_ON(!crtc->primary)? Lack of crtc->primary breaks the world: fbdev stops working, most boot splashes and simplistic userspace stops working, the entire legacy uapi stops working. If the hw can at all do it, we'll require it. So I really think we should require the MUST here (and the warning on top perhaps, or the more elaborate validation I proposed). > > > > + * userspace too much. > > > > I think it would also be useful for atomic userspace. Sure, atomic > > userspace can be expected handle failures, but if there is not at least > > one primary type KMS plane available for a CRTC, then userspace > > probably never uses that CRTC which means the end user could be left > > without an output they wanted. > > The reason why I explicitly mentionned legacy user-space here is that the whole > paragraph is about the internal legacy primary/cursor pointers. I don't want to > mix in requirements for atomic user-space in here. > > But yes some atomic user-space will misbehave if it finds a CRTC without any > candidate primary plane. I guess we could add a new paragraph among those > lines: > > Each CRTC should be compatible with at least one primary plane to avoid > surprising userspace too much. > > But it's not enough, can't have a single primary plane for two CRTCs. > > Each CRTC should be compatible with at least one primary plane, and there > should be as many primary planes as there are CRTCs to avoid suprising > userspace too much. > > But it's not enough, can't have two CRTCs with the same primary plane. Well, > I give up, it's just simpler to use Daniel's criteria. Yeah, also with the validation check we'll now real quick if any driver gets it wrong. Then I think we can have a useful discussion about why, and what to do with that case. As-is we're kinda drafting specs in the void, which is always a bit tough ... That's kinda another reason for doing the stricter check I proposed, it's easier to check and guarantee (on both the driver and compositor side hopefully). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
