[PATCH v4 0/5] drm/doc: misc documentation improvements

[PATCH v4 0/5] drm/doc: misc documentation improvements

[Simon Ser]

Hm, a git rebase was missing some of my changes for v3. Sorry about that.

Simon Ser (5):
  drm/doc: fix drm_plane_type docs
  drm/doc: document the type plane property
  drm/doc: re-format drm.h file comment
  drm/doc: demote old doc-comments in drm.h
  drm/doc: render drm.h uapi docs

 Documentation/gpu/drm-uapi.rst |  3 ++
 drivers/gpu/drm/drm_plane.c    | 58 +++++++++++++++++++--
 include/drm/drm_plane.h        | 21 +++++---
 include/uapi/drm/drm.h         | 93 +++++++++++++++++-----------------
 4 files changed, 116 insertions(+), 59 deletions(-)

-- 
2.29.2

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

[PATCH v4 1/5] 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 1d82b264e5e4..0312a78ffbf1 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.29.2

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

[PATCH v4 2/5] 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,
not that a driver may support cursors without cursor planes (Daniel)

v4: fixing rebase gone wrong

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@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 b15b65e48555..9dce5be5780c 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -49,10 +49,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
@@ -72,6 +70,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_UNIVERSAL_PLANES
+ *     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. Note that
+ *     &DRM_CLIENT_CAP_UNIVERSAL_PLANES is implicitly enabled by
+ *     &DRM_CLIENT_CAP_ATOMIC.
+ *
+ *     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.29.2

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

[PATCH v4 3/5] drm/doc: re-format drm.h file comment

[Simon Ser]

Our documentation build system chokes on \file comments:

    ./include/uapi/drm/drm.h:2: warning: Cannot understand  * \file drm.h
     on line 2 - I thought it was a doc line

Remove all of the slash-directives, and demote to a normal comment. Keep
the historical information because it predates Git.

v3: keep the comment (Daniel)

Signed-off-by: Simon Ser <contact@emersion.fr>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 include/uapi/drm/drm.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 5c31aea1f342..a3ee8e01c295 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -1,11 +1,10 @@
-/**
- * \file drm.h
+/*
  * Header for the Direct Rendering Manager
  *
- * \author Rickard E. (Rik) Faith <faith@valinux.com>
+ * Author: Rickard E. (Rik) Faith <faith@valinux.com>
  *
- * \par Acknowledgments:
- * Dec 1999, Richard Henderson <rth@twiddle.net>, move to generic \c cmpxchg.
+ * Acknowledgments:
+ * Dec 1999, Richard Henderson <rth@twiddle.net>, move to generic cmpxchg.
  */
 
 /*
-- 
2.29.2

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

[PATCH v4 4/5] drm/doc: demote old doc-comments in drm.h

[Simon Ser]

Sphinx doesn't like old doc-comments in drm.h and generates warnings
like:

    ./include/uapi/drm/drm.h:87: warning: cannot understand function prototype: 'struct drm_clip_rect '
    ./include/uapi/drm/drm.h:97: warning: cannot understand function prototype: 'struct drm_drawable_info '
    ./include/uapi/drm/drm.h:105: warning: cannot understand function prototype: 'struct drm_tex_region '
    ...

Demote these to regular comments, because converting all of them is
quite a lot of work (also requires documenting all of the struct fields
for instance). Also many of these structures aren't really used by
modern user-space.

We can easily convert these remaining old comments to Sphinx style on a
one-by-one basis.

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

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index a3ee8e01c295..0827037c5484 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -84,7 +84,7 @@ typedef unsigned int drm_context_t;
 typedef unsigned int drm_drawable_t;
 typedef unsigned int drm_magic_t;
 
-/**
+/*
  * Cliprect.
  *
  * \warning: If you change this structure, make sure you change
@@ -100,7 +100,7 @@ struct drm_clip_rect {
 	unsigned short y2;
 };
 
-/**
+/*
  * Drawable information.
  */
 struct drm_drawable_info {
@@ -108,7 +108,7 @@ struct drm_drawable_info {
 	struct drm_clip_rect *rects;
 };
 
-/**
+/*
  * Texture region,
  */
 struct drm_tex_region {
@@ -119,7 +119,7 @@ struct drm_tex_region {
 	unsigned int age;
 };
 
-/**
+/*
  * Hardware lock.
  *
  * The lock structure is a simple cache-line aligned integer.  To avoid
@@ -131,7 +131,7 @@ struct drm_hw_lock {
 	char padding[60];			/**< Pad to cache line */
 };
 
-/**
+/*
  * DRM_IOCTL_VERSION ioctl argument type.
  *
  * \sa drmGetVersion().
@@ -148,7 +148,7 @@ struct drm_version {
 	char __user *desc;	  /**< User-space buffer to hold desc */
 };
 
-/**
+/*
  * DRM_IOCTL_GET_UNIQUE ioctl argument type.
  *
  * \sa drmGetBusid() and drmSetBusId().
@@ -167,7 +167,7 @@ struct drm_block {
 	int unused;
 };
 
-/**
+/*
  * DRM_IOCTL_CONTROL ioctl argument type.
  *
  * \sa drmCtlInstHandler() and drmCtlUninstHandler().
@@ -182,7 +182,7 @@ struct drm_control {
 	int irq;
 };
 
-/**
+/*
  * Type of memory to map.
  */
 enum drm_map_type {
@@ -194,7 +194,7 @@ enum drm_map_type {
 	_DRM_CONSISTENT = 5	  /**< Consistent memory for PCI DMA */
 };
 
-/**
+/*
  * Memory mapping flags.
  */
 enum drm_map_flags {
@@ -213,7 +213,7 @@ struct drm_ctx_priv_map {
 	void *handle;		 /**< Handle of map */
 };
 
-/**
+/*
  * DRM_IOCTL_GET_MAP, DRM_IOCTL_ADD_MAP and DRM_IOCTL_RM_MAP ioctls
  * argument type.
  *
@@ -230,7 +230,7 @@ struct drm_map {
 	/*   Private data */
 };
 
-/**
+/*
  * DRM_IOCTL_GET_CLIENT ioctl argument type.
  */
 struct drm_client {
@@ -262,7 +262,7 @@ enum drm_stat_type {
 	    /* Add to the *END* of the list */
 };
 
-/**
+/*
  * DRM_IOCTL_GET_STATS ioctl argument type.
  */
 struct drm_stats {
@@ -273,7 +273,7 @@ struct drm_stats {
 	} data[15];
 };
 
-/**
+/*
  * Hardware locking flags.
  */
 enum drm_lock_flags {
@@ -288,7 +288,7 @@ enum drm_lock_flags {
 	_DRM_HALT_CUR_QUEUES = 0x20  /**< Halt all current queues */
 };
 
-/**
+/*
  * DRM_IOCTL_LOCK, DRM_IOCTL_UNLOCK and DRM_IOCTL_FINISH ioctl argument type.
  *
  * \sa drmGetLock() and drmUnlock().
@@ -298,7 +298,7 @@ struct drm_lock {
 	enum drm_lock_flags flags;
 };
 
-/**
+/*
  * DMA flags
  *
  * \warning
@@ -327,7 +327,7 @@ enum drm_dma_flags {
 	_DRM_DMA_LARGER_OK = 0x40     /**< Larger-than-requested buffers OK */
 };
 
-/**
+/*
  * DRM_IOCTL_ADD_BUFS and DRM_IOCTL_MARK_BUFS ioctl argument type.
  *
  * \sa drmAddBufs().
@@ -350,7 +350,7 @@ struct drm_buf_desc {
 				  */
 };
 
-/**
+/*
  * DRM_IOCTL_INFO_BUFS ioctl argument type.
  */
 struct drm_buf_info {
@@ -358,7 +358,7 @@ struct drm_buf_info {
 	struct drm_buf_desc __user *list;
 };
 
-/**
+/*
  * DRM_IOCTL_FREE_BUFS ioctl argument type.
  */
 struct drm_buf_free {
@@ -366,7 +366,7 @@ struct drm_buf_free {
 	int __user *list;
 };
 
-/**
+/*
  * Buffer information
  *
  * \sa drm_buf_map.
@@ -378,7 +378,7 @@ struct drm_buf_pub {
 	void __user *address;	       /**< Address of buffer */
 };
 
-/**
+/*
  * DRM_IOCTL_MAP_BUFS ioctl argument type.
  */
 struct drm_buf_map {
@@ -391,7 +391,7 @@ struct drm_buf_map {
 	struct drm_buf_pub __user *list;	/**< Buffer information */
 };
 
-/**
+/*
  * DRM_IOCTL_DMA ioctl argument type.
  *
  * Indices here refer to the offset into the buffer list in drm_buf_get.
@@ -416,7 +416,7 @@ enum drm_ctx_flags {
 	_DRM_CONTEXT_2DONLY = 0x02
 };
 
-/**
+/*
  * DRM_IOCTL_ADD_CTX ioctl argument type.
  *
  * \sa drmCreateContext() and drmDestroyContext().
@@ -426,7 +426,7 @@ struct drm_ctx {
 	enum drm_ctx_flags flags;
 };
 
-/**
+/*
  * DRM_IOCTL_RES_CTX ioctl argument type.
  */
 struct drm_ctx_res {
@@ -434,14 +434,14 @@ struct drm_ctx_res {
 	struct drm_ctx __user *contexts;
 };
 
-/**
+/*
  * DRM_IOCTL_ADD_DRAW and DRM_IOCTL_RM_DRAW ioctl argument type.
  */
 struct drm_draw {
 	drm_drawable_t handle;
 };
 
-/**
+/*
  * DRM_IOCTL_UPDATE_DRAW ioctl argument type.
  */
 typedef enum {
@@ -455,14 +455,14 @@ struct drm_update_draw {
 	unsigned long long data;
 };
 
-/**
+/*
  * DRM_IOCTL_GET_MAGIC and DRM_IOCTL_AUTH_MAGIC ioctl argument type.
  */
 struct drm_auth {
 	drm_magic_t magic;
 };
 
-/**
+/*
  * DRM_IOCTL_IRQ_BUSID ioctl argument type.
  *
  * \sa drmGetInterruptFromBusID().
@@ -504,7 +504,7 @@ struct drm_wait_vblank_reply {
 	long tval_usec;
 };
 
-/**
+/*
  * DRM_IOCTL_WAIT_VBLANK ioctl argument type.
  *
  * \sa drmWaitVBlank().
@@ -517,7 +517,7 @@ union drm_wait_vblank {
 #define _DRM_PRE_MODESET 1
 #define _DRM_POST_MODESET 2
 
-/**
+/*
  * DRM_IOCTL_MODESET_CTL ioctl argument type
  *
  * \sa drmModesetCtl().
@@ -527,7 +527,7 @@ struct drm_modeset_ctl {
 	__u32 cmd;
 };
 
-/**
+/*
  * DRM_IOCTL_AGP_ENABLE ioctl argument type.
  *
  * \sa drmAgpEnable().
@@ -536,7 +536,7 @@ struct drm_agp_mode {
 	unsigned long mode;	/**< AGP mode */
 };
 
-/**
+/*
  * DRM_IOCTL_AGP_ALLOC and DRM_IOCTL_AGP_FREE ioctls argument type.
  *
  * \sa drmAgpAlloc() and drmAgpFree().
@@ -548,7 +548,7 @@ struct drm_agp_buffer {
 	unsigned long physical;	/**< Physical used by i810 */
 };
 
-/**
+/*
  * DRM_IOCTL_AGP_BIND and DRM_IOCTL_AGP_UNBIND ioctls argument type.
  *
  * \sa drmAgpBind() and drmAgpUnbind().
@@ -558,7 +558,7 @@ struct drm_agp_binding {
 	unsigned long offset;	/**< In bytes -- will round to page boundary */
 };
 
-/**
+/*
  * DRM_IOCTL_AGP_INFO ioctl argument type.
  *
  * \sa drmAgpVersionMajor(), drmAgpVersionMinor(), drmAgpGetMode(),
@@ -579,7 +579,7 @@ struct drm_agp_info {
 	unsigned short id_device;
 };
 
-/**
+/*
  * DRM_IOCTL_SG_ALLOC ioctl argument type.
  */
 struct drm_scatter_gather {
@@ -587,7 +587,7 @@ struct drm_scatter_gather {
 	unsigned long handle;	/**< Used for mapping / unmapping */
 };
 
-/**
+/*
  * DRM_IOCTL_SET_VERSION ioctl argument type.
  */
 struct drm_set_version {
@@ -597,14 +597,14 @@ struct drm_set_version {
 	int drm_dd_minor;
 };
 
-/** DRM_IOCTL_GEM_CLOSE ioctl argument type */
+/* DRM_IOCTL_GEM_CLOSE ioctl argument type */
 struct drm_gem_close {
 	/** Handle of the object to be closed. */
 	__u32 handle;
 	__u32 pad;
 };
 
-/** DRM_IOCTL_GEM_FLINK ioctl argument type */
+/* DRM_IOCTL_GEM_FLINK ioctl argument type */
 struct drm_gem_flink {
 	/** Handle for the object being named */
 	__u32 handle;
@@ -613,7 +613,7 @@ struct drm_gem_flink {
 	__u32 name;
 };
 
-/** DRM_IOCTL_GEM_OPEN ioctl argument type */
+/* DRM_IOCTL_GEM_OPEN ioctl argument type */
 struct drm_gem_open {
 	/** Name of object being opened */
 	__u32 name;
@@ -651,7 +651,7 @@ struct drm_gem_open {
 #define DRM_CAP_SYNCOBJ		0x13
 #define DRM_CAP_SYNCOBJ_TIMELINE	0x14
 
-/** DRM_IOCTL_GET_CAP ioctl argument type */
+/* DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
 	__u64 capability;
 	__u64 value;
@@ -699,7 +699,7 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
 
-/** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
+/* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
 	__u64 value;
@@ -951,7 +951,7 @@ extern "C" {
 
 #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
 
-/**
+/*
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
  * Generic IOCTLS restart at 0xA0.
@@ -962,7 +962,7 @@ extern "C" {
 #define DRM_COMMAND_BASE                0x40
 #define DRM_COMMAND_END			0xA0
 
-/**
+/*
  * Header for events written back to userspace on the drm fd.  The
  * type defines the type of event, the length specifies the total
  * length of the event (including the header), and user_data is
-- 
2.29.2

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

[PATCH v4 5/5] drm/doc: render drm.h uapi docs

[Simon Ser]

It doesn't seem like drm.h docs are included anywhere. Render them next
to drm_mode.h, under the "Userspace API Structures" section.

This also allows references to e.g. DRM_CAP_* to be properly linkified
elsewhere in our docs.

Signed-off-by: Simon Ser <contact@emersion.fr>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 Documentation/gpu/drm-uapi.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 7dce175f6d75..04bdc7a91d53 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -457,5 +457,8 @@ Userspace API Structures
 .. kernel-doc:: include/uapi/drm/drm_mode.h
    :doc: overview
 
+.. kernel-doc:: include/uapi/drm/drm.h
+   :internal:
+
 .. kernel-doc:: include/uapi/drm/drm_mode.h
    :internal:
-- 
2.29.2

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

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

[Daniel Vetter]

On Tue, Dec 22, 2020 at 02:35:21PM +0100, Simon Ser wrote:
> Add a new entry for "type" in the section for standard plane properties.
> 
> v3: improve paragraph about mixing legacy IOCTLs with explicit usage,
> not that a driver may support cursors without cursor planes (Daniel)

s/not/note/

> 
> v4: fixing rebase gone wrong
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@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 b15b65e48555..9dce5be5780c 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -49,10 +49,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
> @@ -72,6 +70,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_UNIVERSAL_PLANES

s/UNIVERSAL_PLANES/ATOMIC/ here?

With just universal planes you don't have atomic test-only. But I guess it
also works as-is, I'm just not entirely clear what you want to state here.

> + *     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. Note that
> + *     &DRM_CLIENT_CAP_UNIVERSAL_PLANES is implicitly enabled by
> + *     &DRM_CLIENT_CAP_ATOMIC.
> + *
> + *     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.

Empty line here for reading comfort in plain text? Same below.

Since you mention formats below, I also wonder whether we should state
here that xrgb8888 is generally supported, worst case through software
emulation. That's defacto the uapi we have to adhere to.

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

Just some thoughts about, feel free to merge as-is.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>   * IN_FORMATS:
>   *     Blob property which contains the set of buffer format and modifier
>   *     pairs supported by this plane. The blob is a struct
> -- 
> 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 v4 2/5] drm/doc: document the type plane property

[Simon Ser]

On Tuesday, December 22nd, 2020 at 2:59 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> > + * type:
> > + *     Immutable property describing the type of the plane.
> > + *
> > + *     For user-space which has enabled the &DRM_CLIENT_CAP_UNIVERSAL_PLANES
>
> s/UNIVERSAL_PLANES/ATOMIC/ here?
>
> With just universal planes you don't have atomic test-only. But I guess it
> also works as-is, I'm just not entirely clear what you want to state here.

Right. This paragraph was written when I wasn't aware about ATOMIC implicitly
enabling UNIVERSAL_PLANES. Fixed in v5.

> > + *     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. Note that
> > + *     &DRM_CLIENT_CAP_UNIVERSAL_PLANES is implicitly enabled by
> > + *     &DRM_CLIENT_CAP_ATOMIC.
> > + *
> > + *     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.
>
> Empty line here for reading comfort in plain text? Same below.
>
> Since you mention formats below, I also wonder whether we should state
> here that xrgb8888 is generally supported, worst case through software
> emulation. That's defacto the uapi we have to adhere to.

I wonder. If a new driver decides not to support XRGB8888, that wouldn't be a
kernel regression because it's about new hardware. Do we want to formally lock
future drivers into XRGB8888 support? Or do we want to open the door for a
driver to break this assumption, even if most user-space won't work on the new
hardware?

I guess all of this is mostly theoretical at this point.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Jan 15, 2021 at 12:06 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, December 22nd, 2020 at 2:59 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > > + * type:
> > > + *     Immutable property describing the type of the plane.
> > > + *
> > > + *     For user-space which has enabled the &DRM_CLIENT_CAP_UNIVERSAL_PLANES
> >
> > s/UNIVERSAL_PLANES/ATOMIC/ here?
> >
> > With just universal planes you don't have atomic test-only. But I guess it
> > also works as-is, I'm just not entirely clear what you want to state here.
>
> Right. This paragraph was written when I wasn't aware about ATOMIC implicitly
> enabling UNIVERSAL_PLANES. Fixed in v5.
>
> > > + *     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. Note that
> > > + *     &DRM_CLIENT_CAP_UNIVERSAL_PLANES is implicitly enabled by
> > > + *     &DRM_CLIENT_CAP_ATOMIC.
> > > + *
> > > + *     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.
> >
> > Empty line here for reading comfort in plain text? Same below.
> >
> > Since you mention formats below, I also wonder whether we should state
> > here that xrgb8888 is generally supported, worst case through software
> > emulation. That's defacto the uapi we have to adhere to.
>
> I wonder. If a new driver decides not to support XRGB8888, that wouldn't be a
> kernel regression because it's about new hardware. Do we want to formally lock
> future drivers into XRGB8888 support? Or do we want to open the door for a
> driver to break this assumption, even if most user-space won't work on the new
> hardware?
>
> I guess all of this is mostly theoretical at this point.

It's been practical since a few years, since a lot of the simple
panels we've added don't support xrgb8888. That's why we have the
fairly good horror show of format conversions in drm_format_helper.c.
And we've done this because too much general distro userspace just
keels over real bad if xrgb8888 doesn't work.
-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