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
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
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?
*/};
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.
*/};
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.
*/};
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!
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Leandro Ribeiro -
Pekka Paalanen -
Simon Ser