Re: [PATCH] drm: rework description of primary and cursor planes

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5099 bytes --]

On Wed, 9 Dec 2020 01:42:23 +0100
Daniel Vetter <daniel@ffwll.ch> 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@emersion.fr>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > ---
> >  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/?

> > + * 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.

Besides, in the other email thread Daniel said:

On Wed, 9 Dec 2020 01:36:37 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> possible_crtcs for a primary plane has exactly the same constraints as
> possible_crtcs for any other plane. The only additional constraint there
> is that:
> - first primary plane you iterate must have the first bit set in
>   possible_crtcs, and it is the primary plane for that crtc
> - 2nd primary plane has the 2nd bit set in possible_crtcs, and it is the
>   primary plane for that crtc
> 

This implies the "must" I suggest above.

Note, that I have no context for this patch here, so I cannot know if
this is documenting a legacy-only KMS struct or legacy+atomic struct.
If the context here is legacy-only, then my comments above need to be
addressed somewhere else that has legacy+atomic context.

Likewise, I have no idea if any certain member or variable in the
kernel is legacy, atomic, or both.

> >   */  
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I think maybe a follow up patch should document how userspace should
> figure out how to line up the primary planes with the right crtcs (if it
> wishes to know that information, it's not super useful aside from probably
> good choice for a fullscreen fallback plane). See my reply on the old
> thread.
> 
> And that patch should also add the code to drm_mode_config_validate() to
> validate the possible_crtc masks for these. Something like
> 
> 	num_primary = 0; num_cursor = 0;
> 
> 	for_each_plane(plane) {
> 		if (plane->type == primary) {
> 			WARN_ON(!(plane->possible_crtcs & BIT(num_primary)));
> 			num_primary++;
> 		}
> 
> 		/* same for cursor */
> 	}
> 
> 	WARN_ON(num_primary != dev->mode_config.num_crtcs);
> }

A good idea.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel