[PATCH v3 0/2] Document drm_mode_get_plane

[PATCH v3 0/2] Document drm_mode_get_plane

[Leandro Ribeiro]

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä <ville.syrjala@linux.intel.com>

v3: document how userspace should find out CRTC index. Also,
document that field 'gamma_size' represents the number of
entries in the lookup table. Suggested by Pekka Paalanen
<ppaalanen@gmail.com> and Daniel Vetter <daniel@ffwll.ch>

Leandro Ribeiro (2):
  drm/doc: document how userspace should find out CRTC index
  drm/doc: document drm_mode_get_plane

 Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
 drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
 include/uapi/drm/drm.h            |  3 ++-
 include/uapi/drm/drm_mode.h       | 22 ++++++++++++++++++++++
 4 files changed, 43 insertions(+), 5 deletions(-)

--
2.31.1

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

[PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

In this patch we add a section to document what userspace should do to
find out the CRTC index. This is important as there are multiple places
in the documentation that need this, so it's better to just point to
this section and avoid repetition.

Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
---
 Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
 drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
 include/uapi/drm/drm.h            |  3 ++-
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 04bdc7a91d53..1aa52a6ac567 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -457,6 +457,20 @@ Userspace API Structures
 .. kernel-doc:: include/uapi/drm/drm_mode.h
    :doc: overview

+.. _crtc_index:
+
+CRTC index
+----------
+
+In some situations, it is important for userspace to find out the index of a
+CRTC. The CRTC index should not be confused with its object id.
+
+In order to do this, userspace should first query the resources object
+from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES
+ioctl). The resources object contains a pointer to an array of CRTC's, and also
+the number of entries of the array. The index of the CRTC is the same as its
+position in this array.
+
 .. kernel-doc:: include/uapi/drm/drm.h
    :internal:

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 3dd70d813f69..9575188d97ee 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -46,10 +46,11 @@
  * it reached a given hardware component (a CRC sampling "source").
  *
  * Userspace can control generation of CRCs in a given CRTC by writing to the
- * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
- * Accepted values are source names (which are driver-specific) and the "auto"
- * keyword, which will let the driver select a default source of frame CRCs
- * for this CRTC.
+ * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the
+ * CRTC. To learn how to find out the index of a certain CRTC, please see
+ * :ref:`crtc_index`. Accepted values are source names (which are
+ * driver-specific) and the "auto" keyword, which will let the driver select a
+ * default source of frame CRCs for this CRTC.
  *
  * Once frame CRC generation is enabled, userspace can capture them by reading
  * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 67b94bc3c885..6944f08ab1a6 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -636,7 +636,8 @@ struct drm_gem_open {
  * DRM_CAP_VBLANK_HIGH_CRTC
  *
  * If set to 1, the kernel supports specifying a CRTC index in the high bits of
- * &drm_wait_vblank_request.type.
+ * &drm_wait_vblank_request.type. To learn how to find out the index of a
+ * certain CRTC, please see :ref:`crtc_index`.
  *
  * Starting kernel version 2.6.39, this capability is always set to 1.
  */
--
2.31.1

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

[PATCH 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
---
 include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a5e76aa06ad5..8fa6495cd948 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,38 @@ struct drm_mode_set_plane {
 	__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ */
 struct drm_mode_get_plane {
+	/** @plane_id: Object ID of the plane. */
 	__u32 plane_id;

+	/** @crtc_id: Object ID of the current CRTC. */
 	__u32 crtc_id;
+	/** @fb_id: Object ID of the current fb. */
 	__u32 fb_id;

+	/**
+	 * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
+	 * are created and they receive an index, which corresponds to their
+	 * position in the bitmask. CRTC with index 0 will be in bit 0, and so
+	 * on. To learn how to find out the index of a certain CRTC, please see
+	 * :ref:`crtc_index`.
+	 */
 	__u32 possible_crtcs;
+	/** @gamma_size: Number of entries of the legacy gamma lookup table. */
 	__u32 gamma_size;

+	/** @count_format_types: Number of formats. */
 	__u32 count_format_types;
+	/**
+	 * @format_type_ptr: Pointer to ``__u32`` array of formats that are
+	 * supported by the plane. These formats do not require modifiers.
+	 */
 	__u64 format_type_ptr;
 };

--
2.31.1

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

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4473 bytes --]

On Wed, 28 Apr 2021 18:36:50 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> In this patch we add a section to document what userspace should do to
> find out the CRTC index. This is important as there are multiple places
> in the documentation that need this, so it's better to just point to
> this section and avoid repetition.
> 
> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
> ---
>  Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
>  include/uapi/drm/drm.h            |  3 ++-
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 04bdc7a91d53..1aa52a6ac567 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -457,6 +457,20 @@ Userspace API Structures
>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>     :doc: overview
> 
> +.. _crtc_index:
> +
> +CRTC index
> +----------
> +
> +In some situations, it is important for userspace to find out the index of a

That could be said about everything, so this sentence has no
information value. Instead, you could start by stating that CRTCs have
both an object ID and an index, and they are not the same thing. CRTC
index is used in cases where a densely packed identifier for a CRTC is
needed, e.g. in bit-for-crtc masks, where using the object ID would not
work.

> +CRTC. The CRTC index should not be confused with its object id.
> +
> +In order to do this, userspace should first query the resources object

Instead of saying what userspace must do, you could just explain where
it can be observed.

> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES

So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
structure with an array of CRTC IDs. CRTC's index is its index in that
array.

> +ioctl). The resources object contains a pointer to an array of CRTC's, and also
> +the number of entries of the array. The index of the CRTC is the same as its
> +position in this array.

Anyway, the idea here is right.

> +
>  .. kernel-doc:: include/uapi/drm/drm.h
>     :internal:
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 3dd70d813f69..9575188d97ee 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -46,10 +46,11 @@
>   * it reached a given hardware component (a CRC sampling "source").
>   *
>   * Userspace can control generation of CRCs in a given CRTC by writing to the
> - * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
> - * Accepted values are source names (which are driver-specific) and the "auto"
> - * keyword, which will let the driver select a default source of frame CRCs
> - * for this CRTC.
> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the
> + * CRTC. To learn how to find out the index of a certain CRTC, please see
> + * :ref:`crtc_index`. Accepted values are source names (which are

This a bit verbose: "To learn..." It could be more concise, like
making the words "the index" a hyperlink instead of adding a whole
sentence.

> + * driver-specific) and the "auto" keyword, which will let the driver select a
> + * default source of frame CRCs for this CRTC.
>   *
>   * Once frame CRC generation is enabled, userspace can capture them by reading
>   * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 67b94bc3c885..6944f08ab1a6 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -636,7 +636,8 @@ struct drm_gem_open {
>   * DRM_CAP_VBLANK_HIGH_CRTC
>   *
>   * If set to 1, the kernel supports specifying a CRTC index in the high bits of
> - * &drm_wait_vblank_request.type.
> + * &drm_wait_vblank_request.type. To learn how to find out the index of a
> + * certain CRTC, please see :ref:`crtc_index`.

The same here with "a CRTC index" turned into a hyperlink.


Thanks,
pq

>   *
>   * Starting kernel version 2.6.39, this capability is always set to 1.
>   */
> --
> 2.31.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/doc: document drm_mode_get_plane

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Wed, 28 Apr 2021 18:36:51 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> Add a small description and document struct fields of
> drm_mode_get_plane.
> 
> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>

Hi,

thanks a lot for revising these.

> ---
>  include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a5e76aa06ad5..8fa6495cd948 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -312,16 +312,38 @@ struct drm_mode_set_plane {
>  	__u32 src_w;
>  };
> 
> +/**
> + * struct drm_mode_get_plane - Get plane metadata.
> + *
> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
> + * plane.
> + */
>  struct drm_mode_get_plane {
> +	/** @plane_id: Object ID of the plane. */

This is an "in" field, right?

"in" meaning that userspace sets it to the ID of the plane it wants
information on.

"out" field is a field written by the kernel as a response.

I'm not sure if the kernel has a habit of documenting these, because we
use libdrm to abstract this so users do not need to care, but I think
it would be nice.

>  	__u32 plane_id;
> 
> +	/** @crtc_id: Object ID of the current CRTC. */
>  	__u32 crtc_id;
> +	/** @fb_id: Object ID of the current fb. */
>  	__u32 fb_id;
> 
> +	/**
> +	 * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
> +	 * are created and they receive an index, which corresponds to their
> +	 * position in the bitmask. CRTC with index 0 will be in bit 0, and so
> +	 * on. To learn how to find out the index of a certain CRTC, please see
> +	 * :ref:`crtc_index`.

This could be shortened to something like bit N corresponds to CRTC
index N, and make "CRTC index N" a hyperlink.

> +	 */
>  	__u32 possible_crtcs;
> +	/** @gamma_size: Number of entries of the legacy gamma lookup table. */
>  	__u32 gamma_size;
> 
> +	/** @count_format_types: Number of formats. */
>  	__u32 count_format_types;
> +	/**
> +	 * @format_type_ptr: Pointer to ``__u32`` array of formats that are
> +	 * supported by the plane. These formats do not require modifiers.
> +	 */
>  	__u64 format_type_ptr;

The count/ptr fields have an interesting usage pattern, which I suppose
is common for all DRM ioctls. Makes me wonder if it should be documented.

AFAIU, count is in+out field: when set to 0, the kernel uses it to
return the count needed. Then userspace allocates space and calls the
ioctl again with the right count and ptr set to point to the allocated
array of count elements. This is so that kernel never allocates memory
on behalf of userspace for the return data, making things much simpler
at the cost of maybe needing to call the ioctl twice to first figure
out long the array should be.

This can be seen in libdrm code for drmModeGetPlane().

There is certainly no point in explaining all that here, that is too
much. But if there was a way to annotate the count member as in+out,
that would be nice. And the ptr member as "in".


Thanks,
pq

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

On 5/6/21 5:50 AM, Pekka Paalanen wrote:
> On Wed, 28 Apr 2021 18:36:50 -0300
> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> 
>> In this patch we add a section to document what userspace should do to
>> find out the CRTC index. This is important as there are multiple places
>> in the documentation that need this, so it's better to just point to
>> this section and avoid repetition.
>>
>> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
>> ---
>>  Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
>>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
>>  include/uapi/drm/drm.h            |  3 ++-
>>  3 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>> index 04bdc7a91d53..1aa52a6ac567 100644
>> --- a/Documentation/gpu/drm-uapi.rst
>> +++ b/Documentation/gpu/drm-uapi.rst
>> @@ -457,6 +457,20 @@ Userspace API Structures
>>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>>     :doc: overview
>>
>> +.. _crtc_index:
>> +
>> +CRTC index
>> +----------
>> +
>> +In some situations, it is important for userspace to find out the index of a
> 
> That could be said about everything, so this sentence has no
> information value. Instead, you could start by stating that CRTCs have
> both an object ID and an index, and they are not the same thing. CRTC
> index is used in cases where a densely packed identifier for a CRTC is
> needed, e.g. in bit-for-crtc masks, where using the object ID would not
> work.
>
>> +CRTC. The CRTC index should not be confused with its object id.
>> +
>> +In order to do this, userspace should first query the resources object
> 
> Instead of saying what userspace must do, you could just explain where
> it can be observed.
> 
>> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES
> 
> So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
> structure with an array of CRTC IDs. CRTC's index is its index in that
> array.
> 
>> +ioctl). The resources object contains a pointer to an array of CRTC's, and also
>> +the number of entries of the array. The index of the CRTC is the same as its
>> +position in this array.
> 
> Anyway, the idea here is right.
> 

So what about:

CRTC's have both an object ID and an index, and they should not be
confused. The index is used in cases where a densely packed identifier
for a CRTC is needed, for instance in a bitmask of CRTC's. (maybe a link
to the possible_crtcs field of struct drm_mode_get_plane? as example)

DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC
id's, and the CRTC index is its position in this array.

>> +
>>  .. kernel-doc:: include/uapi/drm/drm.h
>>     :internal:
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
>> index 3dd70d813f69..9575188d97ee 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -46,10 +46,11 @@
>>   * it reached a given hardware component (a CRC sampling "source").
>>   *
>>   * Userspace can control generation of CRCs in a given CRTC by writing to the
>> - * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
>> - * Accepted values are source names (which are driver-specific) and the "auto"
>> - * keyword, which will let the driver select a default source of frame CRCs
>> - * for this CRTC.
>> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the
>> + * CRTC. To learn how to find out the index of a certain CRTC, please see
>> + * :ref:`crtc_index`. Accepted values are source names (which are
> 
> This a bit verbose: "To learn..." It could be more concise, like
> making the words "the index" a hyperlink instead of adding a whole
> sentence.
> 

Nice, I'll apply this change.

>> + * driver-specific) and the "auto" keyword, which will let the driver select a
>> + * default source of frame CRCs for this CRTC.
>>   *
>>   * Once frame CRC generation is enabled, userspace can capture them by reading
>>   * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 67b94bc3c885..6944f08ab1a6 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -636,7 +636,8 @@ struct drm_gem_open {
>>   * DRM_CAP_VBLANK_HIGH_CRTC
>>   *
>>   * If set to 1, the kernel supports specifying a CRTC index in the high bits of
>> - * &drm_wait_vblank_request.type.
>> + * &drm_wait_vblank_request.type. To learn how to find out the index of a
>> + * certain CRTC, please see :ref:`crtc_index`.
> 
> The same here with "a CRTC index" turned into a hyperlink.
> 

I'll change this as well.

Thanks,
Leandro

> 
> Thanks,
> pq
> 
>>   *
>>   * Starting kernel version 2.6.39, this capability is always set to 1.
>>   */
>> --
>> 2.31.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

On Wed, 12 May 2021 09:50:14 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> On 5/6/21 5:50 AM, Pekka Paalanen wrote:
> > On Wed, 28 Apr 2021 18:36:50 -0300
> > Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> >   
> >> In this patch we add a section to document what userspace should do to
> >> find out the CRTC index. This is important as there are multiple places
> >> in the documentation that need this, so it's better to just point to
> >> this section and avoid repetition.
> >>
> >> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
> >> ---
> >>  Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
> >>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
> >>  include/uapi/drm/drm.h            |  3 ++-
> >>  3 files changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> >> index 04bdc7a91d53..1aa52a6ac567 100644
> >> --- a/Documentation/gpu/drm-uapi.rst
> >> +++ b/Documentation/gpu/drm-uapi.rst
> >> @@ -457,6 +457,20 @@ Userspace API Structures
> >>  .. kernel-doc:: include/uapi/drm/drm_mode.h
> >>     :doc: overview
> >>
> >> +.. _crtc_index:
> >> +
> >> +CRTC index
> >> +----------
> >> +
> >> +In some situations, it is important for userspace to find out the index of a  
> > 
> > That could be said about everything, so this sentence has no
> > information value. Instead, you could start by stating that CRTCs have
> > both an object ID and an index, and they are not the same thing. CRTC
> > index is used in cases where a densely packed identifier for a CRTC is
> > needed, e.g. in bit-for-crtc masks, where using the object ID would not
> > work.
> >  
> >> +CRTC. The CRTC index should not be confused with its object id.
> >> +
> >> +In order to do this, userspace should first query the resources object  
> > 
> > Instead of saying what userspace must do, you could just explain where
> > it can be observed.
> >   
> >> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES  
> > 
> > So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
> > structure with an array of CRTC IDs. CRTC's index is its index in that
> > array.
> >   
> >> +ioctl). The resources object contains a pointer to an array of CRTC's, and also
> >> +the number of entries of the array. The index of the CRTC is the same as its
> >> +position in this array.  
> > 
> > Anyway, the idea here is right.
> >   
> 
> So what about:
> 
> CRTC's have both an object ID and an index, and they should not be
> confused. The index is used in cases where a densely packed identifier
> for a CRTC is needed, for instance in a bitmask of CRTC's. (maybe a link
> to the possible_crtcs field of struct drm_mode_get_plane? as example)
> 
> DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC
> id's, and the CRTC index is its position in this array.

Sure, sounds good.

Capitalized 'ID'?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

On 5/12/21 10:04 AM, Pekka Paalanen wrote:
> On Wed, 12 May 2021 09:50:14 -0300
> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> 
>> On 5/6/21 5:50 AM, Pekka Paalanen wrote:
>>> On Wed, 28 Apr 2021 18:36:50 -0300
>>> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
>>>   
>>>> In this patch we add a section to document what userspace should do to
>>>> find out the CRTC index. This is important as there are multiple places
>>>> in the documentation that need this, so it's better to just point to
>>>> this section and avoid repetition.
>>>>
>>>> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
>>>> ---
>>>>  Documentation/gpu/drm-uapi.rst    | 14 ++++++++++++++
>>>>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +++++----
>>>>  include/uapi/drm/drm.h            |  3 ++-
>>>>  3 files changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>>>> index 04bdc7a91d53..1aa52a6ac567 100644
>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>> @@ -457,6 +457,20 @@ Userspace API Structures
>>>>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>>>>     :doc: overview
>>>>
>>>> +.. _crtc_index:
>>>> +
>>>> +CRTC index
>>>> +----------
>>>> +
>>>> +In some situations, it is important for userspace to find out the index of a  
>>>
>>> That could be said about everything, so this sentence has no
>>> information value. Instead, you could start by stating that CRTCs have
>>> both an object ID and an index, and they are not the same thing. CRTC
>>> index is used in cases where a densely packed identifier for a CRTC is
>>> needed, e.g. in bit-for-crtc masks, where using the object ID would not
>>> work.
>>>  
>>>> +CRTC. The CRTC index should not be confused with its object id.
>>>> +
>>>> +In order to do this, userspace should first query the resources object  
>>>
>>> Instead of saying what userspace must do, you could just explain where
>>> it can be observed.
>>>   
>>>> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES  
>>>
>>> So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
>>> structure with an array of CRTC IDs. CRTC's index is its index in that
>>> array.
>>>   
>>>> +ioctl). The resources object contains a pointer to an array of CRTC's, and also
>>>> +the number of entries of the array. The index of the CRTC is the same as its
>>>> +position in this array.  
>>>
>>> Anyway, the idea here is right.
>>>   
>>
>> So what about:
>>
>> CRTC's have both an object ID and an index, and they should not be
>> confused. The index is used in cases where a densely packed identifier
>> for a CRTC is needed, for instance in a bitmask of CRTC's. (maybe a link
>> to the possible_crtcs field of struct drm_mode_get_plane? as example)
>>
>> DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC
>> id's, and the CRTC index is its position in this array.
> 
> Sure, sounds good.
> 
> Capitalized 'ID'?
> 

Sure, I'll add this change.

Thanks,
Leandro

Re: [PATCH 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 5/6/21 6:10 AM, Pekka Paalanen wrote:
> On Wed, 28 Apr 2021 18:36:51 -0300
> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> 
>> Add a small description and document struct fields of
>> drm_mode_get_plane.
>>
>> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>
> 
> Hi,
> 
> thanks a lot for revising these.
> 
>> ---
>>  include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index a5e76aa06ad5..8fa6495cd948 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -312,16 +312,38 @@ struct drm_mode_set_plane {
>>  	__u32 src_w;
>>  };
>>
>> +/**
>> + * struct drm_mode_get_plane - Get plane metadata.
>> + *
>> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
>> + * plane.
>> + */
>>  struct drm_mode_get_plane {
>> +	/** @plane_id: Object ID of the plane. */
> 
> This is an "in" field, right?
> 
> "in" meaning that userspace sets it to the ID of the plane it wants
> information on.
> 
> "out" field is a field written by the kernel as a response.
> 
> I'm not sure if the kernel has a habit of documenting these, because we
> use libdrm to abstract this so users do not need to care, but I think
> it would be nice.
> 

In a quick look, I couldn't find anything. But I can change the phrasing
to the following:

"@plane_id: Object ID of the plane whose information should be
retrieved. IN field, set by userspace."

>>  	__u32 plane_id;
>>
>> +	/** @crtc_id: Object ID of the current CRTC. */
>>  	__u32 crtc_id;
>> +	/** @fb_id: Object ID of the current fb. */
>>  	__u32 fb_id;
>>
>> +	/**
>> +	 * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
>> +	 * are created and they receive an index, which corresponds to their
>> +	 * position in the bitmask. CRTC with index 0 will be in bit 0, and so
>> +	 * on. To learn how to find out the index of a certain CRTC, please see
>> +	 * :ref:`crtc_index`.
> 
> This could be shortened to something like bit N corresponds to CRTC
> index N, and make "CRTC index N" a hyperlink.
> 

Nice, I'll apply this change.

>> +	 */
>>  	__u32 possible_crtcs;
>> +	/** @gamma_size: Number of entries of the legacy gamma lookup table. */
>>  	__u32 gamma_size;
>>
>> +	/** @count_format_types: Number of formats. */
>>  	__u32 count_format_types;
>> +	/**
>> +	 * @format_type_ptr: Pointer to ``__u32`` array of formats that are
>> +	 * supported by the plane. These formats do not require modifiers.
>> +	 */
>>  	__u64 format_type_ptr;
> 
> The count/ptr fields have an interesting usage pattern, which I suppose
> is common for all DRM ioctls. Makes me wonder if it should be documented.
> 
> AFAIU, count is in+out field: when set to 0, the kernel uses it to
> return the count needed. Then userspace allocates space and calls the
> ioctl again with the right count and ptr set to point to the allocated
> array of count elements. This is so that kernel never allocates memory
> on behalf of userspace for the return data, making things much simpler
> at the cost of maybe needing to call the ioctl twice to first figure
> out long the array should be.
> 
> This can be seen in libdrm code for drmModeGetPlane().
>
> There is certainly no point in explaining all that here, that is too
> much. But if there was a way to annotate the count member as in+out,
> that would be nice. And the ptr member as "in".
> 

This is documented in the description of struct drm_mode_get_connector:

https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#c.drm_mode_get_connector

Would be enough to have something similar in struct drm_mode_get_plane?

> 
> Thanks,
> pq
> 
>>  };
>>
>> --
>> 2.31.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 2/2] drm/doc: document drm_mode_get_plane

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]

On Wed, 19 May 2021 10:30:40 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> On 5/6/21 6:10 AM, Pekka Paalanen wrote:
> > On Wed, 28 Apr 2021 18:36:51 -0300
> > Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> >   
> >> Add a small description and document struct fields of
> >> drm_mode_get_plane.
> >>
> >> Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com>  
> > 
> > Hi,
> > 
> > thanks a lot for revising these.
> >   
> >> ---
> >>  include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index a5e76aa06ad5..8fa6495cd948 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -312,16 +312,38 @@ struct drm_mode_set_plane {
> >>  	__u32 src_w;
> >>  };
> >>
> >> +/**
> >> + * struct drm_mode_get_plane - Get plane metadata.
> >> + *
> >> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
> >> + * plane.
> >> + */
> >>  struct drm_mode_get_plane {
> >> +	/** @plane_id: Object ID of the plane. */  
> > 
> > This is an "in" field, right?
> > 
> > "in" meaning that userspace sets it to the ID of the plane it wants
> > information on.
> > 
> > "out" field is a field written by the kernel as a response.
> > 
> > I'm not sure if the kernel has a habit of documenting these, because we
> > use libdrm to abstract this so users do not need to care, but I think
> > it would be nice.
> >   
> 
> In a quick look, I couldn't find anything. But I can change the phrasing
> to the following:
> 
> "@plane_id: Object ID of the plane whose information should be
> retrieved. IN field, set by userspace."

I think "set by userspace" or "set by caller" would be enough.


> >>  	__u32 plane_id;
> >>
> >> +	/** @crtc_id: Object ID of the current CRTC. */
> >>  	__u32 crtc_id;
> >> +	/** @fb_id: Object ID of the current fb. */
> >>  	__u32 fb_id;
> >>
> >> +	/**
> >> +	 * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
> >> +	 * are created and they receive an index, which corresponds to their
> >> +	 * position in the bitmask. CRTC with index 0 will be in bit 0, and so
> >> +	 * on. To learn how to find out the index of a certain CRTC, please see
> >> +	 * :ref:`crtc_index`.  
> > 
> > This could be shortened to something like bit N corresponds to CRTC
> > index N, and make "CRTC index N" a hyperlink.
> >   
> 
> Nice, I'll apply this change.
> 
> >> +	 */
> >>  	__u32 possible_crtcs;
> >> +	/** @gamma_size: Number of entries of the legacy gamma lookup table. */
> >>  	__u32 gamma_size;
> >>
> >> +	/** @count_format_types: Number of formats. */
> >>  	__u32 count_format_types;
> >> +	/**
> >> +	 * @format_type_ptr: Pointer to ``__u32`` array of formats that are
> >> +	 * supported by the plane. These formats do not require modifiers.
> >> +	 */
> >>  	__u64 format_type_ptr;  
> > 
> > The count/ptr fields have an interesting usage pattern, which I suppose
> > is common for all DRM ioctls. Makes me wonder if it should be documented.
> > 
> > AFAIU, count is in+out field: when set to 0, the kernel uses it to
> > return the count needed. Then userspace allocates space and calls the
> > ioctl again with the right count and ptr set to point to the allocated
> > array of count elements. This is so that kernel never allocates memory
> > on behalf of userspace for the return data, making things much simpler
> > at the cost of maybe needing to call the ioctl twice to first figure
> > out long the array should be.
> > 
> > This can be seen in libdrm code for drmModeGetPlane().
> >
> > There is certainly no point in explaining all that here, that is too
> > much. But if there was a way to annotate the count member as in+out,
> > that would be nice. And the ptr member as "in".
> >   
> 
> This is documented in the description of struct drm_mode_get_connector:
> 
> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#c.drm_mode_get_connector
> 
> Would be enough to have something similar in struct drm_mode_get_plane?

That would be really nice!


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]