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

[Daniel Vetter <daniel@ffwll.ch>]

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@ffwll.ch> wrote:
> 
> > 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/?
> 
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel