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

[Pekka Paalanen <ppaalanen@gmail.com>]

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