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

[Simon Ser]

On Wednesday, December 9th, 2020 at 5:02 PM, Daniel Vetter <daniel at ffwll.ch> wrote:

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

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