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

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

[Simon Ser]

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
+ * userspace too much.
  */
 
 static unsigned int drm_num_planes(struct drm_device *dev)
-- 
2.29.2


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

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

[Daniel Vetter]

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
> + * userspace too much.
>   */

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);
}

Cheers, Daniel

>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> -- 
> 2.29.2
> 
> 

-- 
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

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

[Pekka Paalanen]

[-- 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

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

[Simon Ser]

Thanks for the review!

On Wednesday, December 9th, 2020 at 1:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

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

Yeah, as I've already replied, I won't volunteer to document legacy bits,
documenting atomic is already enough. :P

> 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);
> }

Thanks for the suggestion. However I don't see why a driver should ensure
this. Isn't it perfectly fine for a driver to use primary plane 2 for CRTC 1,
and primary plane 1 for CRTC 2, for the purposes of legacy uAPI?

If we're trying here to invent a new uAPI to let user-space discover the
internal legacy primary/cursor pointers, I'm not signing up for this. The
requirement you're describing seems like something current drivers ensure
"by chance", not an established uAPI. In other words, writing a new driver
that doesn't do the same wouldn't break uAPI contracts.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Dec 09, 2020 at 03:58:17PM +0000, Simon Ser wrote:
> Thanks for the review!
> 
> On Wednesday, December 9th, 2020 at 1:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > 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.
> 
> Yeah, as I've already replied, I won't volunteer to document legacy bits,
> documenting atomic is already enough. :P

This is also somewhat useful as a hint for atomic userspace.

> > 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);
> > }
> 
> Thanks for the suggestion. However I don't see why a driver should ensure
> this. Isn't it perfectly fine for a driver to use primary plane 2 for CRTC 1,
> and primary plane 1 for CRTC 2, for the purposes of legacy uAPI?

Yeah but it's a mess. Messes don't make good uapi.

> If we're trying here to invent a new uAPI to let user-space discover the
> internal legacy primary/cursor pointers, I'm not signing up for this. The
> requirement you're describing seems like something current drivers ensure
> "by chance", not an established uAPI. In other words, writing a new driver
> that doesn't do the same wouldn't break uAPI contracts.

I'm more seeing this as general uapi lock-down. "Everything goes" doesn't
work great. And it's all the same topic really. Like if your userspace
really doesn't care about what the primary plane is (like you seem to
imply), then ofc none of this matters to you, but then also your doc patch
here doesn't matter.
-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

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

[Simon Ser]

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

> On Wed, Dec 09, 2020 at 03:58:17PM +0000, Simon Ser wrote:
> > Thanks for the review!
> >
> > On Wednesday, December 9th, 2020 at 1:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > 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.
> >
> > Yeah, as I've already replied, I won't volunteer to document legacy bits,
> > documenting atomic is already enough. :P
>
> This is also somewhat useful as a hint for atomic userspace.

How so? Atomic can just pick the first compatible primary plane for CRTC 1,
the first available primary plane from the rest for CRTC 2, and so on.

> > > 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);
> > > }
> >
> > Thanks for the suggestion. However I don't see why a driver should ensure
> > this. Isn't it perfectly fine for a driver to use primary plane 2 for CRTC 1,
> > and primary plane 1 for CRTC 2, for the purposes of legacy uAPI?
>
> Yeah but it's a mess. Messes don't make good uapi.
>
> > If we're trying here to invent a new uAPI to let user-space discover the
> > internal legacy primary/cursor pointers, I'm not signing up for this. The
> > requirement you're describing seems like something current drivers ensure
> > "by chance", not an established uAPI. In other words, writing a new driver
> > that doesn't do the same wouldn't break uAPI contracts.
>
> I'm more seeing this as general uapi lock-down. "Everything goes" doesn't
> work great. And it's all the same topic really. Like if your userspace
> really doesn't care about what the primary plane is (like you seem to
> imply), then ofc none of this matters to you, but then also your doc patch
> here doesn't matter.

My patch makes it clear the "one primary plane per CRTC" requirement is just
for legacy uAPI. See the commit message.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Ser]

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)?

> > > + * 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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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

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

[Simon Ser]

On Wednesday, December 9th, 2020 at 8:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> > 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).

Hmm, actually, I'm already hitting a driver which doesn't guarantee that.
amdgpu with my hardware [1] has the first primary plane linked to the the last
CRTC, the second primary plane linked to the second-to-last CRTC, and so on.

[1]: https://drmdb.emersion.fr/devices/129e158a4d9f
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Ser]

Additional note, I don't really want to add the same check for cursor
planes, because I don't want to forbid a driver from having the CRTC
without a cursor plane and the second CRTC with a cursor plane. I don't
know if such heterogeneous hardware exists, but it sounds like
something we should be able to support.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, Dec 10, 2020 at 4:45 PM Simon Ser <contact@emersion.fr> wrote:
> On Wednesday, December 9th, 2020 at 8:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 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).
>
> Hmm, actually, I'm already hitting a driver which doesn't guarantee that.
> amdgpu with my hardware [1] has the first primary plane linked to the the last
> CRTC, the second primary plane linked to the second-to-last CRTC, and so on.
>
> [1]: https://drmdb.emersion.fr/devices/129e158a4d9f

Huh so crtc are registered forward and planes backward? I guess adding
amd people. And yeah sounds like defacto you can't figure out which
primary plane goes to which crtc, and we just take whatever goes.
Maybe that stricter approach with more guarantees just doesn't work,
ship sailed already :-/
-Daniel

-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

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

[Simon Ser]

On Thursday, December 10th, 2020 at 4:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> Huh so crtc are registered forward and planes backward? I guess adding
> amd people. And yeah sounds like defacto you can't figure out which
> primary plane goes to which crtc, and we just take whatever goes.
> Maybe that stricter approach with more guarantees just doesn't work,
> ship sailed already :-/

Yeah. Even if we fixed the amdgpu driver and added the check, user-space still
couldn't have the guarantee that it can associate the n-th primary plane with
the n-th CRTC, because it might run with an old kernel.

If we really wanted to allow user-space to discover the internal
->{primary,cursor} pointers, I think we should just expose a new property. That
way, the uAPI would be a lot more explicit and a lot less guessing. The cost is
that it wouldn't work on older kernels, but with amdgpu user-space can't rely
on the implicit rule you've suggested anyways.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Thu, Dec 10, 2020 at 10:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Dec 10, 2020 at 4:45 PM Simon Ser <contact@emersion.fr> wrote:
> > On Wednesday, December 9th, 2020 at 8:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > 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).
> >
> > Hmm, actually, I'm already hitting a driver which doesn't guarantee that.
> > amdgpu with my hardware [1] has the first primary plane linked to the the last
> > CRTC, the second primary plane linked to the second-to-last CRTC, and so on.
> >
> > [1]: https://drmdb.emersion.fr/devices/129e158a4d9f
>
> Huh so crtc are registered forward and planes backward? I guess adding
> amd people. And yeah sounds like defacto you can't figure out which
> primary plane goes to which crtc, and we just take whatever goes.
> Maybe that stricter approach with more guarantees just doesn't work,
> ship sailed already :-/

IIRC, we used to register them both the same way, but ended up
reversing the order at some point because the direct mapping didn't
work for some reason.  This was years ago though, so the details are
hazy.  Maybe Harry or Leo remembers more details?

Alex

> -Daniel
>
> -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