[PATCH v5 1/2] drm/doc: fix drm_plane_type docs

[PATCH v5 1/2] drm/doc: fix drm_plane_type docs

[Simon Ser]

The docs for enum drm_plane_type mention legacy IOCTLs, however the
plane type is not tied to legacy IOCTLs, the drm_cursor.primary and
cursor fields are. Add a small paragraph to reference these.

Instead, document expectations for primary and cursor planes for
non-legacy userspace. Note that these docs are for driver developers,
not userspace developers, so internal kernel APIs are mentionned.

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 include/drm/drm_plane.h | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 8ef06ee1c8eb..95ab14a4336a 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -538,10 +538,14 @@ struct drm_plane_funcs {
  *
  * For compatibility with legacy userspace, only overlay planes are made
  * available to userspace by default. Userspace clients may set the
- * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
+ * &DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
  * wish to receive a universal plane list containing all plane types. See also
  * drm_for_each_legacy_plane().
  *
+ * In addition to setting each plane's type, drivers need to setup the
+ * &drm_crtc.primary and optionally &drm_crtc.cursor pointers for legacy
+ * IOCTLs. See drm_crtc_init_with_planes().
+ *
  * WARNING: The values of this enum is UABI since they're exposed in the "type"
  * property.
  */
@@ -557,19 +561,20 @@ enum drm_plane_type {
 	/**
 	 * @DRM_PLANE_TYPE_PRIMARY:
 	 *
-	 * Primary planes represent a "main" plane for a CRTC.  Primary planes
-	 * are the planes operated upon by CRTC modesetting and flipping
-	 * operations described in the &drm_crtc_funcs.page_flip and
-	 * &drm_crtc_funcs.set_config hooks.
+	 * A primary plane attached to a CRTC is the most likely to be able to
+	 * light up the CRTC when no scaling/cropping is used and the plane
+	 * covers the whole CRTC.
 	 */
 	DRM_PLANE_TYPE_PRIMARY,
 
 	/**
 	 * @DRM_PLANE_TYPE_CURSOR:
 	 *
-	 * Cursor planes represent a "cursor" plane for a CRTC.  Cursor planes
-	 * are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and
-	 * DRM_IOCTL_MODE_CURSOR2 IOCTLs.
+	 * A cursor plane attached to a CRTC is more likely to be able to be
+	 * enabled when no scaling/cropping is used and the framebuffer has the
+	 * size indicated by &drm_mode_config.cursor_width and
+	 * &drm_mode_config.cursor_height. Additionally, if the driver doesn't
+	 * support modifiers, the framebuffer should have a linear layout.
 	 */
 	DRM_PLANE_TYPE_CURSOR,
 };
-- 
2.30.0

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

[PATCH v5 2/2] drm/doc: document the type plane property

[Simon Ser]

Add a new entry for "type" in the section for standard plane properties.

v3: improve paragraph about mixing legacy IOCTLs with explicit usage,
note that a driver may support cursors without cursor planes (Daniel)

v4: fixing rebase gone wrong

v5:
- Fix typo (Daniel)
- Mention CAP_ATOMIC instead of CAP_UNIVERSAL_PLANES when referring to
  atomic test-only commits (Daniel)
- Add newlines at end of sections (Daniel)

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 drivers/gpu/drm/drm_plane.c | 58 ++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index bf6e525bb116..5affcc7f065b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -50,10 +50,8 @@
  * &struct drm_plane (possibly as part of a larger structure) and registers it
  * with a call to drm_universal_plane_init().
  *
- * The type of a plane is exposed in the immutable "type" enumeration property,
- * 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.
+ * Each plane has a type, see enum drm_plane_type. A plane can be compatible
+ * with multiple CRTCs, see &drm_plane.possible_crtcs.
  *
  * Each CRTC must have a unique primary plane userspace can attach to enable
  * the CRTC. In other words, userspace must be able to attach a different
@@ -73,6 +71,58 @@
  *
  * DRM planes have a few standardized properties:
  *
+ * type:
+ *     Immutable property describing the type of the plane.
+ *
+ *     For user-space which has enabled the &DRM_CLIENT_CAP_ATOMIC capability,
+ *     the plane type is just a hint and is mostly superseded by atomic
+ *     test-only commits. The type hint can still be used to come up more
+ *     easily with a plane configuration accepted by the driver.
+ *
+ *     The value of this property can be one of the following:
+ *
+ *     "Primary":
+ *         To light up a CRTC, attaching a primary plane is the most likely to
+ *         work if it covers the whole CRTC and doesn't have scaling or
+ *         cropping set up.
+ *
+ *         Drivers may support more features for the primary plane, user-space
+ *         can find out with test-only atomic commits.
+ *
+ *         Primary planes are implicitly used by the kernel in the legacy
+ *         IOCTLs &DRM_IOCTL_MODE_SETCRTC and &DRM_IOCTL_MODE_PAGE_FLIP.
+ *         Therefore user-space must not mix explicit usage of any primary
+ *         plane (e.g. through an atomic commit) with these legacy IOCTLs.
+ *
+ *     "Cursor":
+ *         To enable this plane, using a framebuffer configured without scaling
+ *         or cropping and with the following properties is the most likely to
+ *         work:
+ *
+ *         - If the driver provides the capabilities &DRM_CAP_CURSOR_WIDTH and
+ *           &DRM_CAP_CURSOR_HEIGHT, create the framebuffer with this size.
+ *           Otherwise, create a framebuffer with the size 64x64.
+ *         - If the driver doesn't support modifiers, create a framebuffer with
+ *           a linear layout. Otherwise, use the IN_FORMATS plane property.
+ *
+ *         Drivers may support more features for the cursor plane, user-space
+ *         can find out with test-only atomic commits.
+ *
+ *         Cursor planes are implicitly used by the kernel in the legacy
+ *         IOCTLs &DRM_IOCTL_MODE_CURSOR and &DRM_IOCTL_MODE_CURSOR2.
+ *         Therefore user-space must not mix explicit usage of any cursor
+ *         plane (e.g. through an atomic commit) with these legacy IOCTLs.
+ *
+ *         Some drivers may support cursors even if no cursor plane is exposed.
+ *         In this case, the legacy cursor IOCTLs can be used to configure the
+ *         cursor.
+ *
+ *     "Overlay":
+ *         Neither primary nor cursor.
+ *
+ *         Overlay planes are the only planes exposed when the
+ *         &DRM_CLIENT_CAP_UNIVERSAL_PLANES capability is disabled.
+ *
  * IN_FORMATS:
  *     Blob property which contains the set of buffer format and modifier
  *     pairs supported by this plane. The blob is a struct
-- 
2.30.0

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

Re: [PATCH v5 2/2] drm/doc: document the type plane property

[Pekka Paalanen]

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

On Fri, 15 Jan 2021 12:06:26 +0100
Simon Ser <contact@emersion.fr> wrote:

> Add a new entry for "type" in the section for standard plane properties.
> 
> v3: improve paragraph about mixing legacy IOCTLs with explicit usage,
> note that a driver may support cursors without cursor planes (Daniel)
> 
> v4: fixing rebase gone wrong
> 
> v5:
> - Fix typo (Daniel)
> - Mention CAP_ATOMIC instead of CAP_UNIVERSAL_PLANES when referring to
>   atomic test-only commits (Daniel)
> - Add newlines at end of sections (Daniel)
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 58 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index bf6e525bb116..5affcc7f065b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -50,10 +50,8 @@
>   * &struct drm_plane (possibly as part of a larger structure) and registers it
>   * with a call to drm_universal_plane_init().
>   *
> - * The type of a plane is exposed in the immutable "type" enumeration property,
> - * 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.
> + * Each plane has a type, see enum drm_plane_type. A plane can be compatible
> + * with multiple CRTCs, see &drm_plane.possible_crtcs.

Hi,

this part is kernel dev doc, right?

>   *
>   * Each CRTC must have a unique primary plane userspace can attach to enable
>   * the CRTC. In other words, userspace must be able to attach a different
> @@ -73,6 +71,58 @@
>   *
>   * DRM planes have a few standardized properties:
>   *
> + * type:
> + *     Immutable property describing the type of the plane.
> + *
> + *     For user-space which has enabled the &DRM_CLIENT_CAP_ATOMIC capability,
> + *     the plane type is just a hint and is mostly superseded by atomic
> + *     test-only commits. The type hint can still be used to come up more
> + *     easily with a plane configuration accepted by the driver.
> + *
> + *     The value of this property can be one of the following:
> + *
> + *     "Primary":
> + *         To light up a CRTC, attaching a primary plane is the most likely to
> + *         work if it covers the whole CRTC and doesn't have scaling or
> + *         cropping set up.
> + *
> + *         Drivers may support more features for the primary plane, user-space
> + *         can find out with test-only atomic commits.
> + *
> + *         Primary planes are implicitly used by the kernel in the legacy

s/Primary planes/Some primary planes/ perhaps? That would give the
justification for the below "user-space must not" sentence as there is
vagueness in what exactly happens with legacy.

Ok either way.

> + *         IOCTLs &DRM_IOCTL_MODE_SETCRTC and &DRM_IOCTL_MODE_PAGE_FLIP.
> + *         Therefore user-space must not mix explicit usage of any primary
> + *         plane (e.g. through an atomic commit) with these legacy IOCTLs.
> + *
> + *     "Cursor":
> + *         To enable this plane, using a framebuffer configured without scaling
> + *         or cropping and with the following properties is the most likely to
> + *         work:
> + *
> + *         - If the driver provides the capabilities &DRM_CAP_CURSOR_WIDTH and
> + *           &DRM_CAP_CURSOR_HEIGHT, create the framebuffer with this size.
> + *           Otherwise, create a framebuffer with the size 64x64.
> + *         - If the driver doesn't support modifiers, create a framebuffer with
> + *           a linear layout. Otherwise, use the IN_FORMATS plane property.
> + *
> + *         Drivers may support more features for the cursor plane, user-space
> + *         can find out with test-only atomic commits.
> + *
> + *         Cursor planes are implicitly used by the kernel in the legacy

s/Cursor planes/Some cursor planes/ like earlier?

> + *         IOCTLs &DRM_IOCTL_MODE_CURSOR and &DRM_IOCTL_MODE_CURSOR2.
> + *         Therefore user-space must not mix explicit usage of any cursor
> + *         plane (e.g. through an atomic commit) with these legacy IOCTLs.
> + *
> + *         Some drivers may support cursors even if no cursor plane is exposed.
> + *         In this case, the legacy cursor IOCTLs can be used to configure the
> + *         cursor.
> + *
> + *     "Overlay":
> + *         Neither primary nor cursor.
> + *
> + *         Overlay planes are the only planes exposed when the
> + *         &DRM_CLIENT_CAP_UNIVERSAL_PLANES capability is disabled.

This is a bit terse. Can we say anything more about overlay planes?
Even just "nothing is guaranteed, use test_only commits to find out
what works"?

> + *
>   * IN_FORMATS:
>   *     Blob property which contains the set of buffer format and modifier
>   *     pairs supported by this plane. The blob is a struct

In any case,
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>


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 v5 1/2] drm/doc: fix drm_plane_type docs

[Pekka Paalanen]

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

On Fri, 15 Jan 2021 12:06:25 +0100
Simon Ser <contact@emersion.fr> wrote:

> The docs for enum drm_plane_type mention legacy IOCTLs, however the
> plane type is not tied to legacy IOCTLs, the drm_cursor.primary and
> cursor fields are. Add a small paragraph to reference these.
> 
> Instead, document expectations for primary and cursor planes for
> non-legacy userspace. Note that these docs are for driver developers,
> not userspace developers, so internal kernel APIs are mentionned.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> ---
>  include/drm/drm_plane.h | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8ef06ee1c8eb..95ab14a4336a 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -538,10 +538,14 @@ struct drm_plane_funcs {
>   *
>   * For compatibility with legacy userspace, only overlay planes are made
>   * available to userspace by default. Userspace clients may set the
> - * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
> + * &DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
>   * wish to receive a universal plane list containing all plane types. See also
>   * drm_for_each_legacy_plane().
>   *
> + * In addition to setting each plane's type, drivers need to setup the
> + * &drm_crtc.primary and optionally &drm_crtc.cursor pointers for legacy
> + * IOCTLs. See drm_crtc_init_with_planes().
> + *
>   * WARNING: The values of this enum is UABI since they're exposed in the "type"
>   * property.
>   */
> @@ -557,19 +561,20 @@ enum drm_plane_type {
>  	/**
>  	 * @DRM_PLANE_TYPE_PRIMARY:
>  	 *
> -	 * Primary planes represent a "main" plane for a CRTC.  Primary planes
> -	 * are the planes operated upon by CRTC modesetting and flipping
> -	 * operations described in the &drm_crtc_funcs.page_flip and
> -	 * &drm_crtc_funcs.set_config hooks.
> +	 * A primary plane attached to a CRTC is the most likely to be able to
> +	 * light up the CRTC when no scaling/cropping is used and the plane
> +	 * covers the whole CRTC.
>  	 */
>  	DRM_PLANE_TYPE_PRIMARY,
>  
>  	/**
>  	 * @DRM_PLANE_TYPE_CURSOR:
>  	 *
> -	 * Cursor planes represent a "cursor" plane for a CRTC.  Cursor planes
> -	 * are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and
> -	 * DRM_IOCTL_MODE_CURSOR2 IOCTLs.
> +	 * A cursor plane attached to a CRTC is more likely to be able to be
> +	 * enabled when no scaling/cropping is used and the framebuffer has the
> +	 * size indicated by &drm_mode_config.cursor_width and
> +	 * &drm_mode_config.cursor_height. Additionally, if the driver doesn't
> +	 * support modifiers, the framebuffer should have a linear layout.

Hi,

is there anything to be said about positioning a cursor plane partially
off-screen?

>  	 */
>  	DRM_PLANE_TYPE_CURSOR,
>  };

Anyway,
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


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 v5 1/2] drm/doc: fix drm_plane_type docs

[Daniel Vetter]

On Mon, Jan 18, 2021 at 10:40:52AM +0200, Pekka Paalanen wrote:
> On Fri, 15 Jan 2021 12:06:25 +0100
> Simon Ser <contact@emersion.fr> wrote:
> 
> > The docs for enum drm_plane_type mention legacy IOCTLs, however the
> > plane type is not tied to legacy IOCTLs, the drm_cursor.primary and
> > cursor fields are. Add a small paragraph to reference these.
> > 
> > Instead, document expectations for primary and cursor planes for
> > non-legacy userspace. Note that these docs are for driver developers,
> > not userspace developers, so internal kernel APIs are mentionned.
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > ---
> >  include/drm/drm_plane.h | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8ef06ee1c8eb..95ab14a4336a 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -538,10 +538,14 @@ struct drm_plane_funcs {
> >   *
> >   * For compatibility with legacy userspace, only overlay planes are made
> >   * available to userspace by default. Userspace clients may set the
> > - * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
> > + * &DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
> >   * wish to receive a universal plane list containing all plane types. See also
> >   * drm_for_each_legacy_plane().
> >   *
> > + * In addition to setting each plane's type, drivers need to setup the
> > + * &drm_crtc.primary and optionally &drm_crtc.cursor pointers for legacy
> > + * IOCTLs. See drm_crtc_init_with_planes().
> > + *
> >   * WARNING: The values of this enum is UABI since they're exposed in the "type"
> >   * property.
> >   */
> > @@ -557,19 +561,20 @@ enum drm_plane_type {
> >  	/**
> >  	 * @DRM_PLANE_TYPE_PRIMARY:
> >  	 *
> > -	 * Primary planes represent a "main" plane for a CRTC.  Primary planes
> > -	 * are the planes operated upon by CRTC modesetting and flipping
> > -	 * operations described in the &drm_crtc_funcs.page_flip and
> > -	 * &drm_crtc_funcs.set_config hooks.
> > +	 * A primary plane attached to a CRTC is the most likely to be able to
> > +	 * light up the CRTC when no scaling/cropping is used and the plane
> > +	 * covers the whole CRTC.
> >  	 */
> >  	DRM_PLANE_TYPE_PRIMARY,
> >  
> >  	/**
> >  	 * @DRM_PLANE_TYPE_CURSOR:
> >  	 *
> > -	 * Cursor planes represent a "cursor" plane for a CRTC.  Cursor planes
> > -	 * are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and
> > -	 * DRM_IOCTL_MODE_CURSOR2 IOCTLs.
> > +	 * A cursor plane attached to a CRTC is more likely to be able to be
> > +	 * enabled when no scaling/cropping is used and the framebuffer has the
> > +	 * size indicated by &drm_mode_config.cursor_width and
> > +	 * &drm_mode_config.cursor_height. Additionally, if the driver doesn't
> > +	 * support modifiers, the framebuffer should have a linear layout.
> 
> Hi,
> 
> is there anything to be said about positioning a cursor plane partially
> off-screen?

It should work, like anything partially off-screen placed plane. But
there's two issues:
- you might run into hw limitations (and uh there's even hw where this
  holds for the cursor plane, specifically cursor on 3rd crtc on
  cherrytrail i915 is broken and we can't do some of the placements you'd
  want from a cursor because the display block would die).
- there's still a bunch of drivers which don't even clip correctly :-/

Iow, it's a bit a mess ...
-Daniel

> 
> >  	 */
> >  	DRM_PLANE_TYPE_CURSOR,
> >  };
> 
> Anyway,
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> 
> 
> Thanks,
> pq



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