| Message ID | 20210422181004.34247-2-leandro.ribeiro@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Document drm_mode_get_plane | expand |
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
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 >
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 > >
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
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!
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
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
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!
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; };
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(+) -- 2.31.1