[PATCH v2 0/1] Document drm_mode_get_plane

[PATCH v2 0/1] Document drm_mode_get_plane

[Leandro Ribeiro]

This is a follow up of the patchset "Document how userspace should use
plane format list and IN_FORMATS":

https://lists.freedesktop.org/archives/dri-devel/2021-April/302433.html

The first patch of the series ("drm/doc: document drm_mode_get_plane")
is still useful, although the other commit of the series was incorrect.
So I'm pushing the first commit again.

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

Leandro Ribeiro (1):
  drm/doc: document drm_mode_get_plane

 include/uapi/drm/drm_mode.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--
2.31.1

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

[PATCH v2 1/1] 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a5e76aa06ad5..3e85de928db9 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,32 @@ 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. */
 	__u32 possible_crtcs;
+	/** @gamma_size: Size of the legacy gamma 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 v2 1/1] drm/doc: document drm_mode_get_plane

[Pekka Paalanen]

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

On Thu, 22 Apr 2021 15:10:04 -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>
> ---
>  include/uapi/drm/drm_mode.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a5e76aa06ad5..3e85de928db9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -312,16 +312,32 @@ 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. */

This should probably explain what the bits in the mask correspond to.
As in, which CRTC does bit 0 refer to, and so on.

>  	__u32 possible_crtcs;
> +	/** @gamma_size: Size of the legacy gamma table. */

What are the units? Entries? Bytes?

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

I wonder if the "do not require modifiers" is again going too far in
making a difference between this list and IN_FORMATS?

> +	 */
>  	__u64 format_type_ptr;
>  };

Other than those, looks like a significant improvement to me.


Thanks,
pq

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


[-- 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 v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/23/21 8:11 AM, Pekka Paalanen wrote:
> On Thu, 22 Apr 2021 15:10:04 -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>
>> ---
>>  include/uapi/drm/drm_mode.h | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index a5e76aa06ad5..3e85de928db9 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -312,16 +312,32 @@ 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. */
> 
> This should probably explain what the bits in the mask correspond to.
> As in, which CRTC does bit 0 refer to, and so on.
> 

What about:

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

>>  	__u32 possible_crtcs;
>> +	/** @gamma_size: Size of the legacy gamma table. */
> 
> What are the units? Entries? Bytes?
> 

The number of entries. I'll update to "gamma_size: Number of entries of
the legacy gamma lookup table" in the next version.

>>  	__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.
> 
> I wonder if the "do not require modifiers" is again going too far in
> making a difference between this list and IN_FORMATS?
> 

Yes that's true, I'll drop this phrase.

>> +	 */
>>  	__u64 format_type_ptr;
>>  };
> 
> Other than those, looks like a significant improvement to me.
> 
> 
> Thanks,
> pq
> 
>>
>> --
>> 2.31.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Pekka Paalanen]

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

On Fri, 23 Apr 2021 18:30:33 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> On 4/23/21 8:11 AM, Pekka Paalanen wrote:
> > On Thu, 22 Apr 2021 15:10:04 -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>
> >> ---
> >>  include/uapi/drm/drm_mode.h | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index a5e76aa06ad5..3e85de928db9 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -312,16 +312,32 @@ 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. */  
> > 
> > This should probably explain what the bits in the mask correspond to.
> > As in, which CRTC does bit 0 refer to, and so on.
> >   
> 
> What about:
> 
> "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."

This would still need to explain where can I find this index.

> >>  	__u32 possible_crtcs;
> >> +	/** @gamma_size: Size of the legacy gamma table. */  
> > 
> > What are the units? Entries? Bytes?
> >   
> 
> The number of entries. I'll update to "gamma_size: Number of entries of
> the legacy gamma lookup table" in the next version.

Sounds good!


Thanks,
pq

> >>  	__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.  
> > 
> > I wonder if the "do not require modifiers" is again going too far in
> > making a difference between this list and IN_FORMATS?
> >   
> 
> Yes that's true, I'll drop this phrase.
> 
> >> +	 */
> >>  	__u64 format_type_ptr;
> >>  };  
> > 
> > Other than those, looks like a significant improvement to me.
> > 
> > 
> > Thanks,
> > pq
> >   
> >>
> >> --
> >> 2.31.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> >   


[-- 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 v2 1/1] drm/doc: document drm_mode_get_plane

[Simon Ser]

On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> > > This should probably explain what the bits in the mask correspond to.
> > > As in, which CRTC does bit 0 refer to, and so on.
> >
> > What about:
> >
> > "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."
>
> This would still need to explain where can I find this index.

This closed merge request had some docs about possible CRTCs:

https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/26/21 7:58 AM, Simon Ser wrote:
> On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
>>>> This should probably explain what the bits in the mask correspond to.
>>>> As in, which CRTC does bit 0 refer to, and so on.
>>>
>>> What about:
>>>
>>> "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."
>>
>> This would still need to explain where can I find this index.
> 

What do you mean?

> This closed merge request had some docs about possible CRTCs:
> 
> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
> 
I'm afraid I don't know exactly what you expect to be documented here
that is still missing. Could you please elaborate?

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

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Pekka Paalanen]

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

On Mon, 26 Apr 2021 14:30:53 -0300
Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:

> On 4/26/21 7:58 AM, Simon Ser wrote:
> > On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >   
> >>>> This should probably explain what the bits in the mask correspond to.
> >>>> As in, which CRTC does bit 0 refer to, and so on.  
> >>>
> >>> What about:
> >>>
> >>> "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."  
> >>
> >> This would still need to explain where can I find this index.  
> >   
> 
> What do you mean?
> 
> > This closed merge request had some docs about possible CRTCs:
> > 
> > https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
> >   
> I'm afraid I don't know exactly what you expect to be documented here
> that is still missing. Could you please elaborate?
> 
> Thanks a lot!

The documentation you add is talking about "CRTC index". What defines a
CRTC object's index? How do I determine what index a CRTC object has?

The answer is, AFAIK, that the index is never stored explicitly
anywhere. You have to get the DRM resources structure, which has an
array for CRTC IDs. The index is the index to that array, IIRC. So if
one does not already know this, it is going to be really hard to figure
out what the "index" is. It might even be confused with the object ID,
which it is not but the ID might by complete accident be less than 32
so it would look ok at first glance.

If the index is already explained somewhere else, a reference to that
documentation would be enough.


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 v2 1/1] drm/doc: document drm_mode_get_plane

[Daniel Vetter]

On Tue, Apr 27, 2021 at 10:40:24AM +0300, Pekka Paalanen wrote:
> On Mon, 26 Apr 2021 14:30:53 -0300
> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
> 
> > On 4/26/21 7:58 AM, Simon Ser wrote:
> > > On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >   
> > >>>> This should probably explain what the bits in the mask correspond to.
> > >>>> As in, which CRTC does bit 0 refer to, and so on.  
> > >>>
> > >>> What about:
> > >>>
> > >>> "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."  
> > >>
> > >> This would still need to explain where can I find this index.  
> > >   
> > 
> > What do you mean?
> > 
> > > This closed merge request had some docs about possible CRTCs:
> > > 
> > > https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
> > >   
> > I'm afraid I don't know exactly what you expect to be documented here
> > that is still missing. Could you please elaborate?
> > 
> > Thanks a lot!
> 
> The documentation you add is talking about "CRTC index". What defines a
> CRTC object's index? How do I determine what index a CRTC object has?
> 
> The answer is, AFAIK, that the index is never stored explicitly
> anywhere. You have to get the DRM resources structure, which has an
> array for CRTC IDs. The index is the index to that array, IIRC. So if
> one does not already know this, it is going to be really hard to figure
> out what the "index" is. It might even be confused with the object ID,
> which it is not but the ID might by complete accident be less than 32
> so it would look ok at first glance.
> 
> If the index is already explained somewhere else, a reference to that
> documentation would be enough.

I think if we do this we should have a DOC: section in the drm_mode.h uapi
header which explains how the index is computed, and then we reference
that everywhere. Because otherwise there's going to be a _lot_ of
duplication of this all over. Kernel-internally we solve this by just
referencing drm_foo_index() family of functions, but for the uapi there's
really nothing, so needs text.

-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 v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/27/21 6:00 AM, Daniel Vetter wrote:
> On Tue, Apr 27, 2021 at 10:40:24AM +0300, Pekka Paalanen wrote:
>> On Mon, 26 Apr 2021 14:30:53 -0300
>> Leandro Ribeiro <leandro.ribeiro@collabora.com> wrote:
>>
>>> On 4/26/21 7:58 AM, Simon Ser wrote:
>>>> On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>   
>>>>>>> This should probably explain what the bits in the mask correspond to.
>>>>>>> As in, which CRTC does bit 0 refer to, and so on.  
>>>>>>
>>>>>> What about:
>>>>>>
>>>>>> "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."  
>>>>>
>>>>> This would still need to explain where can I find this index.  
>>>>   
>>>
>>> What do you mean?
>>>
>>>> This closed merge request had some docs about possible CRTCs:
>>>>
>>>> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
>>>>   
>>> I'm afraid I don't know exactly what you expect to be documented here
>>> that is still missing. Could you please elaborate?
>>>
>>> Thanks a lot!
>>
>> The documentation you add is talking about "CRTC index". What defines a
>> CRTC object's index? How do I determine what index a CRTC object has?
>>
>> The answer is, AFAIK, that the index is never stored explicitly
>> anywhere. You have to get the DRM resources structure, which has an
>> array for CRTC IDs. The index is the index to that array, IIRC. So if
>> one does not already know this, it is going to be really hard to figure
>> out what the "index" is. It might even be confused with the object ID,
>> which it is not but the ID might by complete accident be less than 32
>> so it would look ok at first glance.
>>
>> If the index is already explained somewhere else, a reference to that
>> documentation would be enough.
> 
> I think if we do this we should have a DOC: section in the drm_mode.h uapi
> header which explains how the index is computed, and then we reference
> that everywhere. Because otherwise there's going to be a _lot_ of
> duplication of this all over. Kernel-internally we solve this by just
> referencing drm_foo_index() family of functions, but for the uapi there's
> really nothing, so needs text.
> 
> -Daniel
>
Ok, I've sent a v3 with a small section to document how to get the index
of a CRTC object from userspace perspective. But I could only find two
comments that would benefit from it (at least in "Userland interfaces"
page).

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