* [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS @ 2021-04-06 19:21 Leandro Ribeiro 2021-04-06 19:21 ` [PATCH 1/2] drm/doc: document drm_mode_get_plane Leandro Ribeiro ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Leandro Ribeiro @ 2021-04-06 19:21 UTC (permalink / raw) To: dri-devel; +Cc: airlied, pekka.paalanen, kernel This patch is to emphasize how userspace should use the plane format list and the IN_FORMATS blob. The plane format list contains the formats that do not require modifiers, and the blob property has the formats that support modifiers. Note that these are not disjoint sets. If a format supports modifiers but the driver can also handle it without a modifier, it should be present in both the IN_FORMATS blob property and the plane format list. This is important for userspace, as there are situations in which we need to find out if the KMS driver can handle a certain format without any modifiers. Leandro Ribeiro (2): drm/doc: document drm_mode_get_plane drm/doc: emphasize difference between plane formats and IN_FORMATS blob drivers/gpu/drm/drm_plane.c | 4 ++++ include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) -- 2.31.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] drm/doc: document drm_mode_get_plane 2021-04-06 19:21 [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Leandro Ribeiro @ 2021-04-06 19:21 ` Leandro Ribeiro 2021-04-07 13:45 ` Ville Syrjälä 2021-04-06 19:21 ` [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob Leandro Ribeiro 2021-04-08 11:35 ` [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Daniel Vetter 2 siblings, 1 reply; 17+ messages in thread From: Leandro Ribeiro @ 2021-04-06 19:21 UTC (permalink / raw) To: dri-devel; +Cc: airlied, pekka.paalanen, kernel 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 | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index d1a93d2a85f9..96fc9a6da608 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -312,16 +312,35 @@ 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: Pointer to ``__u32`` array of CRTC's that are + * 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/doc: document drm_mode_get_plane 2021-04-06 19:21 ` [PATCH 1/2] drm/doc: document drm_mode_get_plane Leandro Ribeiro @ 2021-04-07 13:45 ` Ville Syrjälä 2021-04-08 19:26 ` Leandro Ribeiro 0 siblings, 1 reply; 17+ messages in thread From: Ville Syrjälä @ 2021-04-07 13:45 UTC (permalink / raw) To: Leandro Ribeiro; +Cc: airlied, pekka.paalanen, kernel, dri-devel On Tue, Apr 06, 2021 at 04:21:17PM -0300, Leandro Ribeiro 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 | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index d1a93d2a85f9..96fc9a6da608 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -312,16 +312,35 @@ 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: Pointer to ``__u32`` array of CRTC's that are > + * compatible with the plane. > + */ It's a bitmask. > __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 -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/doc: document drm_mode_get_plane 2021-04-07 13:45 ` Ville Syrjälä @ 2021-04-08 19:26 ` Leandro Ribeiro 2021-04-20 8:51 ` Simon Ser 0 siblings, 1 reply; 17+ messages in thread From: Leandro Ribeiro @ 2021-04-08 19:26 UTC (permalink / raw) To: Ville Syrjälä; +Cc: airlied, pekka.paalanen, kernel, dri-devel On 4/7/21 10:45 AM, Ville Syrjälä wrote: > On Tue, Apr 06, 2021 at 04:21:17PM -0300, Leandro Ribeiro 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 | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >> index d1a93d2a85f9..96fc9a6da608 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -312,16 +312,35 @@ 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: Pointer to ``__u32`` array of CRTC's that are >> + * compatible with the plane. >> + */ > > It's a bitmask. Thank you, I'll fix this in the next version. > >> __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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/doc: document drm_mode_get_plane 2021-04-08 19:26 ` Leandro Ribeiro @ 2021-04-20 8:51 ` Simon Ser 0 siblings, 0 replies; 17+ messages in thread From: Simon Ser @ 2021-04-20 8:51 UTC (permalink / raw) To: Leandro Ribeiro; +Cc: airlied, pekka.paalanen, kernel, dri-devel Hi Leandro, Any chance you could re-spin at least the first patch? Thanks, Simon _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-06 19:21 [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Leandro Ribeiro 2021-04-06 19:21 ` [PATCH 1/2] drm/doc: document drm_mode_get_plane Leandro Ribeiro @ 2021-04-06 19:21 ` Leandro Ribeiro 2021-04-07 13:51 ` Ville Syrjälä 2021-04-08 11:35 ` [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Daniel Vetter 2 siblings, 1 reply; 17+ messages in thread From: Leandro Ribeiro @ 2021-04-06 19:21 UTC (permalink / raw) To: dri-devel; +Cc: airlied, pekka.paalanen, kernel Emphasize how userspace should use the plane format list (format_type_ptr) and the IN_FORMATS blob property. Formats exposed in the plane format list (format_type_ptr) do not require modifiers, and formats that are present in the IN_FORMATS blob property support modifiers. Note that these are not disjoint sets. If a format supports modifiers but the driver can also handle it without a modifier, it should be present in both the IN_FORMATS blob property and the plane format list. Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com> --- drivers/gpu/drm/drm_plane.c | 4 ++++ include/uapi/drm/drm_mode.h | 3 +++ 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 0dd43882fe7c..b48d9bd81a59 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -128,6 +128,10 @@ * pairs supported by this plane. The blob is a struct * drm_format_modifier_blob. Without this property the plane doesn't * support buffers with modifiers. Userspace cannot change this property. + * + * To find out the list of buffer formats which are supported without a + * modifier, userspace should not look at this blob property, but at the + * formats list of the plane: &drm_mode_get_plane.format_type_ptr. */ static unsigned int drm_num_planes(struct drm_device *dev) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 96fc9a6da608..4293800ec095 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -340,6 +340,9 @@ struct drm_mode_get_plane { /** * @format_type_ptr: Pointer to ``__u32`` array of formats that are * supported by the plane. These formats do not require modifiers. + * + * To find out the list of formats that support modifiers, userspace + * must use the plane IN_FORMATS blob property. */ __u64 format_type_ptr; }; -- 2.31.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-06 19:21 ` [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob Leandro Ribeiro @ 2021-04-07 13:51 ` Ville Syrjälä 2021-04-08 8:39 ` Simon Ser 0 siblings, 1 reply; 17+ messages in thread From: Ville Syrjälä @ 2021-04-07 13:51 UTC (permalink / raw) To: Leandro Ribeiro; +Cc: airlied, pekka.paalanen, kernel, dri-devel On Tue, Apr 06, 2021 at 04:21:18PM -0300, Leandro Ribeiro wrote: > Emphasize how userspace should use the plane format list > (format_type_ptr) and the IN_FORMATS blob property. > > Formats exposed in the plane format list (format_type_ptr) do not > require modifiers, and formats that are present in the IN_FORMATS blob > property support modifiers. > > Note that these are not disjoint sets. If a format supports modifiers > but the driver can also handle it without a modifier, it should be > present in both the IN_FORMATS blob property and the plane format list. > > Signed-off-by: Leandro Ribeiro <leandro.ribeiro@collabora.com> > --- > drivers/gpu/drm/drm_plane.c | 4 ++++ > include/uapi/drm/drm_mode.h | 3 +++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 0dd43882fe7c..b48d9bd81a59 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -128,6 +128,10 @@ > * pairs supported by this plane. The blob is a struct > * drm_format_modifier_blob. Without this property the plane doesn't > * support buffers with modifiers. Userspace cannot change this property. > + * > + * To find out the list of buffer formats which are supported without a > + * modifier, userspace should not look at this blob property, but at the > + * formats list of the plane: &drm_mode_get_plane.format_type_ptr. > */ > > static unsigned int drm_num_planes(struct drm_device *dev) > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 96fc9a6da608..4293800ec095 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -340,6 +340,9 @@ struct drm_mode_get_plane { > /** > * @format_type_ptr: Pointer to ``__u32`` array of formats that are > * supported by the plane. These formats do not require modifiers. > + * > + * To find out the list of formats that support modifiers, userspace > + * must use the plane IN_FORMATS blob property. > */ Addfb2+modifiers predates the IN_FORMATS blob, so this doesn't match reality. > __u64 format_type_ptr; > }; > -- > 2.31.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-07 13:51 ` Ville Syrjälä @ 2021-04-08 8:39 ` Simon Ser 2021-04-08 9:59 ` Pekka Paalanen 0 siblings, 1 reply; 17+ messages in thread From: Simon Ser @ 2021-04-08 8:39 UTC (permalink / raw) To: Ville Syrjälä Cc: airlied, pekka.paalanen, kernel, dri-devel, Leandro Ribeiro On Wednesday, April 7th, 2021 at 3:51 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > + * To find out the list of formats that support modifiers, userspace > > + * must use the plane IN_FORMATS blob property. > > */ > > Addfb2+modifiers predates the IN_FORMATS blob, so this doesn't > match reality. TBH, I'm inclined not to care about this edge-case. It's already complicated enough for user-space to figure out what's the right thing to do when supporting both implicit modifiers and explicit modifiers. Using modifiers without IN_FORMATS is risky, since a whole part of the modifier negotiation mechanism is missing. Maybe we can just stick a "since kernel x.y.z" in here to address your concern. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 8:39 ` Simon Ser @ 2021-04-08 9:59 ` Pekka Paalanen 2021-04-08 11:30 ` Daniel Vetter 0 siblings, 1 reply; 17+ messages in thread From: Pekka Paalanen @ 2021-04-08 9:59 UTC (permalink / raw) To: Simon Ser, Ville Syrjälä Cc: airlied, kernel, Leandro Ribeiro, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2723 bytes --] On Thu, 08 Apr 2021 08:39:10 +0000 Simon Ser <contact@emersion.fr> wrote: > On Wednesday, April 7th, 2021 at 3:51 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > + * To find out the list of formats that support modifiers, userspace > > > + * must use the plane IN_FORMATS blob property. > > > */ > > > > Addfb2+modifiers predates the IN_FORMATS blob, so this doesn't > > match reality. > > TBH, I'm inclined not to care about this edge-case. It's already > complicated enough for user-space to figure out what's the right thing > to do when supporting both implicit modifiers and explicit modifiers. > Using modifiers without IN_FORMATS is risky, since a whole part of the > modifier negotiation mechanism is missing. > > Maybe we can just stick a "since kernel x.y.z" in here to address your > concern. Yeah, or we could word it less "must", e.g. "The list of supported formats with their explicit modifiers is advertised via IN_FORMATS blob." We don't have to require userspace to not use the explicit modifier uAPI if IN_FORMATS does not exist. There might be other ways how userspace determines the explicit modifiers, like a Wayland compositor advertising those format-modifier pairs that EGL can import. Then clients use those, and the compositor can try to import those to KMS as well. Maybe it works, maybe it doesn't (the same even if IN_FORMATS exists). IN_FORMATS just gives better chances of picking something that ends up working. The thing userspace *must not* do is to try to use the no-modifiers uAPI when it has an explicit modifier. But do we then have exceptions for MOD_LINEAR? If a buffer has been allocated with explicit modifier MOD_LINEAR, does it mean it is ok to import to KMS using the no-modifiers uAPI or not? The other things userspace *must not* do is use the explicit modifier uAPI when it does not have an explicit modifier, in essence pulling a modifier out of a hat. It might be tempting to use MOD_LINEAR in that case, sometimes it might work, but it's wrong. Userspace must use the no-modifiers uAPI instead. Right? The point of these documentation patches is to establish the convention that: - drm_mode_get_plane::format_type_ptr is the list of pixel formats that can work via the no-modifiers uAPI, but says nothing about the explicit modifiers uAPI. - IN_FORMATS is a list of format-modifier pairs that can work via the explicit modifiers API, but says nothing about the no-modifiers uAPI. Is that a reasonable expectation? Is it also so that passing MOD_INVALID to the explicit modifier uAPI (ADDFB2) is invalid argument? Do we have that documented? 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 9:59 ` Pekka Paalanen @ 2021-04-08 11:30 ` Daniel Vetter 2021-04-08 13:57 ` Pekka Paalanen 0 siblings, 1 reply; 17+ messages in thread From: Daniel Vetter @ 2021-04-08 11:30 UTC (permalink / raw) To: Pekka Paalanen; +Cc: airlied, dri-devel, Leandro Ribeiro, kernel On Thu, Apr 08, 2021 at 12:59:19PM +0300, Pekka Paalanen wrote: > On Thu, 08 Apr 2021 08:39:10 +0000 > Simon Ser <contact@emersion.fr> wrote: > > > On Wednesday, April 7th, 2021 at 3:51 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > + * To find out the list of formats that support modifiers, userspace > > > > + * must use the plane IN_FORMATS blob property. > > > > */ > > > > > > Addfb2+modifiers predates the IN_FORMATS blob, so this doesn't > > > match reality. > > > > TBH, I'm inclined not to care about this edge-case. It's already > > complicated enough for user-space to figure out what's the right thing > > to do when supporting both implicit modifiers and explicit modifiers. > > Using modifiers without IN_FORMATS is risky, since a whole part of the > > modifier negotiation mechanism is missing. > > > > Maybe we can just stick a "since kernel x.y.z" in here to address your > > concern. > > Yeah, or we could word it less "must", e.g. "The list of supported > formats with their explicit modifiers is advertised via IN_FORMATS blob." > > We don't have to require userspace to not use the explicit modifier > uAPI if IN_FORMATS does not exist. There might be other ways how > userspace determines the explicit modifiers, like a Wayland compositor > advertising those format-modifier pairs that EGL can import. Then > clients use those, and the compositor can try to import those to KMS as > well. Maybe it works, maybe it doesn't (the same even if IN_FORMATS > exists). > > IN_FORMATS just gives better chances of picking something that ends up > working. Yup. > The thing userspace *must not* do is to try to use the no-modifiers uAPI > when it has an explicit modifier. But do we then have exceptions for > MOD_LINEAR? > > If a buffer has been allocated with explicit modifier MOD_LINEAR, does > it mean it is ok to import to KMS using the no-modifiers uAPI or not? Maybe it work with current userspace, but not guaranteed. I think we should strongly discourage this at least. The case this can go boom is if userspace allocates a big bo, with some implicit metadata. And then suballocates some linear buffer out of that with explicit modifiers. This could happen if userspace does realize modifiers work, and then does some funky optimization to linear e.g. as part of a resolve pass. Not such hw/driver currently exists, but not something I'd guarantee never happens. If you then create a drm_fb with that with no modifier specified, you get the implicit one from the metadata. > > The other things userspace *must not* do is use the explicit modifier > uAPI when it does not have an explicit modifier, in essence pulling a > modifier out of a hat. It might be tempting to use MOD_LINEAR in that > case, sometimes it might work, but it's wrong. Userspace must use the > no-modifiers uAPI instead. Yes. Userspace can't guess the modifier if it doesn't have it. > > Right? > > The point of these documentation patches is to establish the convention > that: > > - drm_mode_get_plane::format_type_ptr is the list of pixel formats that > can work via the no-modifiers uAPI, but says nothing about the > explicit modifiers uAPI. > > - IN_FORMATS is a list of format-modifier pairs that can work via the > explicit modifiers API, but says nothing about the no-modifiers uAPI. > > Is that a reasonable expectation? I'm not sure. I thought they're the same list underneath in the kernel, at least for drivers that do support modifiers. The current wording I think suggests more meaning than is actually there. > Is it also so that passing MOD_INVALID to the explicit modifier uAPI > (ADDFB2) is invalid argument? Do we have that documented? We'd need to check that, currently it's an out-of-band flag in the struct. Atm DRM_FORMAT_MOD_INVALID is entirely an internal sentinel value to denote end-of-array entries. In practice it wont pass because we validate the modifiers against the advertised list. -Daniel > > > Thanks, > pq > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 11:30 ` Daniel Vetter @ 2021-04-08 13:57 ` Pekka Paalanen 2021-04-08 14:39 ` Ville Syrjälä 0 siblings, 1 reply; 17+ messages in thread From: Pekka Paalanen @ 2021-04-08 13:57 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, dri-devel, Leandro Ribeiro, kernel [-- Attachment #1.1: Type: text/plain, Size: 3432 bytes --] On Thu, 8 Apr 2021 13:30:16 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Apr 08, 2021 at 12:59:19PM +0300, Pekka Paalanen wrote: > > The point of these documentation patches is to establish the convention > > that: > > > > - drm_mode_get_plane::format_type_ptr is the list of pixel formats that > > can work via the no-modifiers uAPI, but says nothing about the > > explicit modifiers uAPI. > > > > - IN_FORMATS is a list of format-modifier pairs that can work via the > > explicit modifiers API, but says nothing about the no-modifiers uAPI. > > > > Is that a reasonable expectation? > > I'm not sure. I thought they're the same list underneath in the kernel, at > least for drivers that do support modifiers. The current wording I think > suggests more meaning than is actually there. They may be the same list in the kernel today, but do you want to force all future drivers and future formats-modifiers to have that too? Or did the boat sail already? The existing uAPI considers these two to be independent lists, no documentation saying otherwise, is there? Should a kernel driver not have a way to say "this format won't work via the no-modifiers uAPI"? The practical consequence in userspace is how should userspace collect the lists of supported format-modifier pairs, when the kernel has two independent format lists, one carries modifiers explicitly and the other does not. The one that carries explicit modifiers cannot denote "no modifier" AFAIU. So the "obvious" interpretation in userspace is that: - the format list that does not carry any modifier information should be used with the no-modifiers uAPI, and - the format list that does carry explicit modifiers should be used with the explicit modifiers uAPI. If you were to say, that if IN_FORMATS exists, use it and ignore the old no-modifiers format list, then the conclusion in userspace when IN_FORMATS exists is that you cannot use the no-modifiers uAPI, because all formats that are listed as supported carry an explicit modifier. I understand that the format or format-modifier lists are not authoritative. Formats outside of the lists *could* work. But why would anyone bother trying something that is not suggested to work? Or, is the intention such, that all formats in IN_FORMATS list imply some support through the no-modifiers uAPI too, iff buffer allocation does not give you an explicit modifier? Or, should there be an i-g-t test to ensure that both the old and IN_FORMATS lists have the exact same pixel formats always, carving that fact into stone and resolving all this ambiguity? > > Is it also so that passing MOD_INVALID to the explicit modifier uAPI > > (ADDFB2) is invalid argument? Do we have that documented? > > We'd need to check that, currently it's an out-of-band flag in the struct. > Atm DRM_FORMAT_MOD_INVALID is entirely an internal sentinel value to > denote end-of-array entries. > > In practice it wont pass because we validate the modifiers against the > advertised list. Right, so while at it, would be good to document that one cannot substitute no-modifiers uAPI with explicit modifier uAPI using MOD_INVALID. This should be documented, because other userspace APIs have tendency to gravitate towards having just one explicit modifiers function allowing MOD_INVALID, meaning "no modifier". 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 13:57 ` Pekka Paalanen @ 2021-04-08 14:39 ` Ville Syrjälä 2021-04-09 9:44 ` Pekka Paalanen 2021-04-12 13:16 ` Daniel Vetter 0 siblings, 2 replies; 17+ messages in thread From: Ville Syrjälä @ 2021-04-08 14:39 UTC (permalink / raw) To: Pekka Paalanen; +Cc: airlied, dri-devel, Leandro Ribeiro, kernel On Thu, Apr 08, 2021 at 04:57:51PM +0300, Pekka Paalanen wrote: > On Thu, 8 Apr 2021 13:30:16 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Thu, Apr 08, 2021 at 12:59:19PM +0300, Pekka Paalanen wrote: > > > > The point of these documentation patches is to establish the convention > > > that: > > > > > > - drm_mode_get_plane::format_type_ptr is the list of pixel formats that > > > can work via the no-modifiers uAPI, but says nothing about the > > > explicit modifiers uAPI. > > > > > > - IN_FORMATS is a list of format-modifier pairs that can work via the > > > explicit modifiers API, but says nothing about the no-modifiers uAPI. > > > > > > Is that a reasonable expectation? > > > > I'm not sure. I thought they're the same list underneath in the kernel, at > > least for drivers that do support modifiers. The current wording I think > > suggests more meaning than is actually there. > > They may be the same list in the kernel today, but do you want to force > all future drivers and future formats-modifiers to have that too? > Or did the boat sail already? > > The existing uAPI considers these two to be independent lists, no > documentation saying otherwise, is there? > > Should a kernel driver not have a way to say "this format won't work > via the no-modifiers uAPI"? > > The practical consequence in userspace is how should userspace collect > the lists of supported format-modifier pairs, when the kernel has two > independent format lists, one carries modifiers explicitly and the > other does not. The one that carries explicit modifiers cannot denote > "no modifier" AFAIU. > > So the "obvious" interpretation in userspace is that: > - the format list that does not carry any modifier information should > be used with the no-modifiers uAPI, and > - the format list that does carry explicit modifiers should be used > with the explicit modifiers uAPI. > > If you were to say, that if IN_FORMATS exists, use it and ignore the > old no-modifiers format list, then the conclusion in userspace when > IN_FORMATS exists is that you cannot use the no-modifiers uAPI, because > all formats that are listed as supported carry an explicit modifier. > > I understand that the format or format-modifier lists are not > authoritative. Formats outside of the lists *could* work. But why would > anyone bother trying something that is not suggested to work? IMO formats not listed by any plane should just be rejected by addfb2. I tried to put that check in the drm core actually but there was some weird pushback, so for the moment it's handled by each driver. Some drivers (like i915) will reject anything not supported by any plane, other drivers might not (and probably no one knows how badly they might blow up if you pass in some exotic format...). I also had some igt patches to test that addfb2 behaviour but they didn't go in either. > > Or, is the intention such, that all formats in IN_FORMATS list imply > some support through the no-modifiers uAPI too, iff buffer > allocation does not give you an explicit modifier? > > Or, should there be an i-g-t test to ensure that both the old and > IN_FORMATS lists have the exact same pixel formats always, carving that > fact into stone and resolving all this ambiguity? > > > > Is it also so that passing MOD_INVALID to the explicit modifier uAPI > > > (ADDFB2) is invalid argument? Do we have that documented? > > > > We'd need to check that, currently it's an out-of-band flag in the struct. > > Atm DRM_FORMAT_MOD_INVALID is entirely an internal sentinel value to > > denote end-of-array entries. > > > > In practice it wont pass because we validate the modifiers against the > > advertised list. We don't actually. If the driver provides the .format_mod_supported() hook then it's up to the driver to validate the modifier in said hook. This was done so that people can embed metadata inside the modifier while only having the base modifier on the modifier list. How userspace is supposed to figure out which values for this extra metadata are valid I have no idea. -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 14:39 ` Ville Syrjälä @ 2021-04-09 9:44 ` Pekka Paalanen 2021-04-12 13:16 ` Daniel Vetter 1 sibling, 0 replies; 17+ messages in thread From: Pekka Paalanen @ 2021-04-09 9:44 UTC (permalink / raw) To: Ville Syrjälä, Daniel Vetter Cc: airlied, kernel, dri-devel, Leandro Ribeiro [-- Attachment #1.1: Type: text/plain, Size: 1556 bytes --] On Thu, 8 Apr 2021 17:39:22 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Apr 08, 2021 at 04:57:51PM +0300, Pekka Paalanen wrote: > > On Thu, 8 Apr 2021 13:30:16 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > Is it also so that passing MOD_INVALID to the explicit modifier uAPI > > > > (ADDFB2) is invalid argument? Do we have that documented? > > > > > > We'd need to check that, currently it's an out-of-band flag in the struct. > > > Atm DRM_FORMAT_MOD_INVALID is entirely an internal sentinel value to > > > denote end-of-array entries. > > > > > > In practice it wont pass because we validate the modifiers against the > > > advertised list. > > We don't actually. If the driver provides the .format_mod_supported() > hook then it's up to the driver to validate the modifier in said hook. > This was done so that people can embed metadata inside the modifier > while only having the base modifier on the modifier list. How userspace > is supposed to figure out which values for this extra metadata are valid > I have no idea. Maybe it's the difference between generic userspace and userspace drivers? I've been having the feeling that these two have different "rules". Maybe that distinction should be formalised in documentation somewhere? Generic userspace never looks into modifiers, it just relays them and compares them as opaque 64-bit words. Userspace drivers are allowed to look into what a modifier actually means and fiddle with it. 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob 2021-04-08 14:39 ` Ville Syrjälä 2021-04-09 9:44 ` Pekka Paalanen @ 2021-04-12 13:16 ` Daniel Vetter 1 sibling, 0 replies; 17+ messages in thread From: Daniel Vetter @ 2021-04-12 13:16 UTC (permalink / raw) To: Ville Syrjälä; +Cc: airlied, Leandro Ribeiro, dri-devel, kernel On Thu, Apr 08, 2021 at 05:39:22PM +0300, Ville Syrjälä wrote: > On Thu, Apr 08, 2021 at 04:57:51PM +0300, Pekka Paalanen wrote: > > On Thu, 8 Apr 2021 13:30:16 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Thu, Apr 08, 2021 at 12:59:19PM +0300, Pekka Paalanen wrote: > > > > > > The point of these documentation patches is to establish the convention > > > > that: > > > > > > > > - drm_mode_get_plane::format_type_ptr is the list of pixel formats that > > > > can work via the no-modifiers uAPI, but says nothing about the > > > > explicit modifiers uAPI. > > > > > > > > - IN_FORMATS is a list of format-modifier pairs that can work via the > > > > explicit modifiers API, but says nothing about the no-modifiers uAPI. > > > > > > > > Is that a reasonable expectation? > > > > > > I'm not sure. I thought they're the same list underneath in the kernel, at > > > least for drivers that do support modifiers. The current wording I think > > > suggests more meaning than is actually there. > > > > They may be the same list in the kernel today, but do you want to force > > all future drivers and future formats-modifiers to have that too? > > Or did the boat sail already? > > > > The existing uAPI considers these two to be independent lists, no > > documentation saying otherwise, is there? > > > > Should a kernel driver not have a way to say "this format won't work > > via the no-modifiers uAPI"? > > > > The practical consequence in userspace is how should userspace collect > > the lists of supported format-modifier pairs, when the kernel has two > > independent format lists, one carries modifiers explicitly and the > > other does not. The one that carries explicit modifiers cannot denote > > "no modifier" AFAIU. > > > > So the "obvious" interpretation in userspace is that: > > - the format list that does not carry any modifier information should > > be used with the no-modifiers uAPI, and > > - the format list that does carry explicit modifiers should be used > > with the explicit modifiers uAPI. Imo the right interpretation is "it's the same list". > > If you were to say, that if IN_FORMATS exists, use it and ignore the > > old no-modifiers format list, then the conclusion in userspace when > > IN_FORMATS exists is that you cannot use the no-modifiers uAPI, because > > all formats that are listed as supported carry an explicit modifier. So formats without modifiers are very funny in their semantics. It means - implied modifier is untiled - except on i915 and radeon/amdgpu, where there's magic hidden information and you might get something else. But it won't work in multi-gpu situations - except bugs (like imo mesa trying to use tiling without modifiers and without magic in-kernel tiling information forwarding is just broken, and surprise, it's broken). We've had some where parts of mesa where assuming modifiers are ok, but that wasn't the case. Now maybe we can expose to userspace which drivers have magic device-local tiling information sharing, but I don't expect this list to ever grow. Anything beyond that sounds like the kernel should maintain a bug list of things which are broken in userspace. > > I understand that the format or format-modifier lists are not > > authoritative. Formats outside of the lists *could* work. But why would > > anyone bother trying something that is not suggested to work? > > IMO formats not listed by any plane should just be rejected by addfb2. > I tried to put that check in the drm core actually but there was some > weird pushback, so for the moment it's handled by each driver. Some > drivers (like i915) will reject anything not supported by any plane, > other drivers might not (and probably no one knows how badly they > might blow up if you pass in some exotic format...). > > I also had some igt patches to test that addfb2 behaviour but > they didn't go in either. > > > Or, is the intention such, that all formats in IN_FORMATS list imply > > some support through the no-modifiers uAPI too, iff buffer > > allocation does not give you an explicit modifier? > > > > Or, should there be an i-g-t test to ensure that both the old and > > IN_FORMATS lists have the exact same pixel formats always, carving that > > fact into stone and resolving all this ambiguity? > > > > > > Is it also so that passing MOD_INVALID to the explicit modifier uAPI > > > > (ADDFB2) is invalid argument? Do we have that documented? > > > > > > We'd need to check that, currently it's an out-of-band flag in the struct. > > > Atm DRM_FORMAT_MOD_INVALID is entirely an internal sentinel value to > > > denote end-of-array entries. > > > > > > In practice it wont pass because we validate the modifiers against the > > > advertised list. > > We don't actually. If the driver provides the .format_mod_supported() > hook then it's up to the driver to validate the modifier in said hook. > This was done so that people can embed metadata inside the modifier > while only having the base modifier on the modifier list. How userspace > is supposed to figure out which values for this extra metadata are valid > I have no idea. It took me a while to figure this out again, but now I remember. It's because of combinatorial explosion of modifiers with stuff like AFBC. So the modifier list is not necessarily an exhaustive list of all combinations, some of the modifiers count more like flags. Which is kinda hilarious since it means userspace can't do anything with modifiers it gets :-/ -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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS 2021-04-06 19:21 [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Leandro Ribeiro 2021-04-06 19:21 ` [PATCH 1/2] drm/doc: document drm_mode_get_plane Leandro Ribeiro 2021-04-06 19:21 ` [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob Leandro Ribeiro @ 2021-04-08 11:35 ` Daniel Vetter 2021-04-08 22:24 ` Leandro Ribeiro 2 siblings, 1 reply; 17+ messages in thread From: Daniel Vetter @ 2021-04-08 11:35 UTC (permalink / raw) To: Leandro Ribeiro; +Cc: airlied, dri-devel, pekka.paalanen, kernel On Tue, Apr 06, 2021 at 04:21:16PM -0300, Leandro Ribeiro wrote: > This patch is to emphasize how userspace should use the plane format list and > the IN_FORMATS blob. The plane format list contains the formats that do not > require modifiers, and the blob property has the formats that support > modifiers. Uh this is a very strong statement that I don't think is supported by kernel driver code. Where is this from. > Note that these are not disjoint sets. If a format supports modifiers but the > driver can also handle it without a modifier, it should be present in both the > IN_FORMATS blob property and the plane format list. Same here ... I thought these two lists are 100% consistent. If not sounds like driver bugs that we need to maybe validate in drm_plane_init. > This is important for userspace, as there are situations in which we need to > find out if the KMS driver can handle a certain format without any modifiers. I don't think you can rely on this. No modifiers means implicit modifier, and the only thing that can give you such buffers is defacto mesa userspace drivers (since that all depends upon driver private magic, with maybe some kernel metadata passed around in private ioctls on the render node). Maybe for more context, what's the problem you've hit and trying to clarify here? -Daniel > > Leandro Ribeiro (2): > drm/doc: document drm_mode_get_plane > drm/doc: emphasize difference between plane formats and IN_FORMATS > blob > > drivers/gpu/drm/drm_plane.c | 4 ++++ > include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > -- > 2.31.1 > -- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS 2021-04-08 11:35 ` [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Daniel Vetter @ 2021-04-08 22:24 ` Leandro Ribeiro 2021-04-12 13:59 ` Daniel Vetter 0 siblings, 1 reply; 17+ messages in thread From: Leandro Ribeiro @ 2021-04-08 22:24 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, pekka.paalanen, kernel, dri-devel On 4/8/21 8:35 AM, Daniel Vetter wrote: > On Tue, Apr 06, 2021 at 04:21:16PM -0300, Leandro Ribeiro wrote: >> This patch is to emphasize how userspace should use the plane format list and >> the IN_FORMATS blob. The plane format list contains the formats that do not >> require modifiers, and the blob property has the formats that support >> modifiers. > > Uh this is a very strong statement that I don't think is supported by > kernel driver code. Where is this from. > >> Note that these are not disjoint sets. If a format supports modifiers but the >> driver can also handle it without a modifier, it should be present in both the >> IN_FORMATS blob property and the plane format list. > > Same here ... > Yes, sorry. The wording was not good. To clarify: I'm trying to document a good approach that userspace *can* (not must) take to be able to tell if a certain format can be used in the pre-modifier kernel uAPI or if it only works with modifiers. The background is that we are reworking the way that Weston stores the formats and modifiers supported by the planes, and there were some wrong assumptions in the code related to what we can assume that the KMS driver supports. We've discussed and decided to send a patch to raise a discussion and check if the conclusions that we've made were reasonable. And if not, what would be a better approach. This is part of a MR in which we add support for the dmabuf-hints protocol extension in Weston. In sort, in Weston we store the formats and modifiers supported by the planes. Then we send them to the client and it may pick one of these format/modifier pairs to allocate its buffers, increasing the chances of its surface ending up in a plane. Here are two commits of the MR that are related to this discussion: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=de6fc18bc35c2e43dff936dd85f310d1f778a7b8 https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=75363bdb121bda2f326109afca5f4c3259423b7d Thanks! > I thought these two lists are 100% consistent. If not sounds like driver > bugs that we need to maybe validate in drm_plane_init. > >> This is important for userspace, as there are situations in which we need to >> find out if the KMS driver can handle a certain format without any modifiers. > > I don't think you can rely on this. No modifiers means implicit modifier, > and the only thing that can give you such buffers is defacto mesa > userspace drivers (since that all depends upon driver private magic, with > maybe some kernel metadata passed around in private ioctls on the render > node). > > Maybe for more context, what's the problem you've hit and trying to > clarify here? > -Daniel > >> >> Leandro Ribeiro (2): >> drm/doc: document drm_mode_get_plane >> drm/doc: emphasize difference between plane formats and IN_FORMATS >> blob >> >> drivers/gpu/drm/drm_plane.c | 4 ++++ >> include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ >> 2 files changed, 26 insertions(+) >> >> -- >> 2.31.1 >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS 2021-04-08 22:24 ` Leandro Ribeiro @ 2021-04-12 13:59 ` Daniel Vetter 0 siblings, 0 replies; 17+ messages in thread From: Daniel Vetter @ 2021-04-12 13:59 UTC (permalink / raw) To: Leandro Ribeiro; +Cc: airlied, dri-devel, pekka.paalanen, kernel On Thu, Apr 08, 2021 at 07:24:30PM -0300, Leandro Ribeiro wrote: > > > On 4/8/21 8:35 AM, Daniel Vetter wrote: > > On Tue, Apr 06, 2021 at 04:21:16PM -0300, Leandro Ribeiro wrote: > >> This patch is to emphasize how userspace should use the plane format list and > >> the IN_FORMATS blob. The plane format list contains the formats that do not > >> require modifiers, and the blob property has the formats that support > >> modifiers. > > > > Uh this is a very strong statement that I don't think is supported by > > kernel driver code. Where is this from. > > > >> Note that these are not disjoint sets. If a format supports modifiers but the > >> driver can also handle it without a modifier, it should be present in both the > >> IN_FORMATS blob property and the plane format list. > > > > Same here ... > > > > Yes, sorry. The wording was not good. To clarify: Ok I think this context helps. > I'm trying to document a good approach that userspace *can* (not must) > take to be able to tell if a certain format can be used in the > pre-modifier kernel uAPI or if it only works with modifiers. I think the short summary is "use modifiers everywhere you can". > The background is that we are reworking the way that Weston stores the > formats and modifiers supported by the planes, and there were some wrong > assumptions in the code related to what we can assume that the KMS > driver supports. > > We've discussed and decided to send a patch to raise a discussion and > check if the conclusions that we've made were reasonable. And if not, > what would be a better approach. > > This is part of a MR in which we add support for the dmabuf-hints > protocol extension in Weston. In sort, in Weston we store the formats > and modifiers supported by the planes. Then we send them to the client > and it may pick one of these format/modifier pairs to allocate its > buffers, increasing the chances of its surface ending up in a plane. > > Here are two commits of the MR that are related to this discussion: > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=de6fc18bc35c2e43dff936dd85f310d1f778a7b8 - drmModePlane's format list (the older, which does not advertise modifiers). Formats exposed through this support implicit modifiers. The above isn't an accurate statement imo. Implied modifiers is a pretty good mess: - On most SoC platforms addfb1 actually implies linear. Except mesa got that wrong in a bunch of cases, and now everyone is unhappy. - On i915/amdgpu/radeon there's implicit modifiers. Maybe also on nouveau I guess, not sure about any of the others. These don't generally work across device instances. - On the kernel side, for drivers supporting modifiers, figuring out which implied modifier to pick is driver specific. There's bugs where essentially depending upon use case things wont work out. - There are currently at least formats which never work with untiled modifier, so essentiall useless on addfb1. This applies to some afbc compressed formats. In short: implied modifier is best effort trying to make stuff work, somewhat, no guarantees. The real recommendation is to not use implied modifiers if you can, so also not use addfb1. > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=75363bdb121bda2f326109afca5f4c3259423b7d Again this isn't reflecting current reality. Right now the kernel puts the same list into both. There is no meaning attached to these two lists being different, they are not. Userspace starting to attach meaning pretty much means we cannot, ever, make them different, and any additional hints would need to be conveyed through new uapi somewhere else, maybe entire new list of formats. Some more fun things around modifiers: - Some formats don't work at all with untile, so useless with addfb1. These got mostly added for afbc support I think. - What's even more fun and I don't think documented anywhere: The modifier list is treated as a bitmask for some drivers, e.g. afbc drivers generally don't list all combinations, but just the flags they support. So you might get a format+modifier combo that's not even in the list you have, and it will actually work (with addfb2) I think before we add new meaning to these two lists and somehow imply they can be different (right now they are never different, in any kernel that shipped ever since modifier support landed) is to document the current modifier rules. Cheers, Daniel > > Thanks! > > > I thought these two lists are 100% consistent. If not sounds like driver > > bugs that we need to maybe validate in drm_plane_init. > > > >> This is important for userspace, as there are situations in which we need to > >> find out if the KMS driver can handle a certain format without any modifiers. > > > > I don't think you can rely on this. No modifiers means implicit modifier, > > and the only thing that can give you such buffers is defacto mesa > > userspace drivers (since that all depends upon driver private magic, with > > maybe some kernel metadata passed around in private ioctls on the render > > node). > > > > Maybe for more context, what's the problem you've hit and trying to > > clarify here? > > -Daniel > > > >> > >> Leandro Ribeiro (2): > >> drm/doc: document drm_mode_get_plane > >> drm/doc: emphasize difference between plane formats and IN_FORMATS > >> blob > >> > >> drivers/gpu/drm/drm_plane.c | 4 ++++ > >> include/uapi/drm/drm_mode.h | 22 ++++++++++++++++++++++ > >> 2 files changed, 26 insertions(+) > >> > >> -- > >> 2.31.1 > >> > > -- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-04-20 8:52 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-06 19:21 [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Leandro Ribeiro 2021-04-06 19:21 ` [PATCH 1/2] drm/doc: document drm_mode_get_plane Leandro Ribeiro 2021-04-07 13:45 ` Ville Syrjälä 2021-04-08 19:26 ` Leandro Ribeiro 2021-04-20 8:51 ` Simon Ser 2021-04-06 19:21 ` [PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob Leandro Ribeiro 2021-04-07 13:51 ` Ville Syrjälä 2021-04-08 8:39 ` Simon Ser 2021-04-08 9:59 ` Pekka Paalanen 2021-04-08 11:30 ` Daniel Vetter 2021-04-08 13:57 ` Pekka Paalanen 2021-04-08 14:39 ` Ville Syrjälä 2021-04-09 9:44 ` Pekka Paalanen 2021-04-12 13:16 ` Daniel Vetter 2021-04-08 11:35 ` [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS Daniel Vetter 2021-04-08 22:24 ` Leandro Ribeiro 2021-04-12 13:59 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.