drm: add support for tiled/compressed/etc modifier in addfb2

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

TODO move definition of tokens to drm_fourcc.h?

Signed-off-by Rob Clark <[email protected]>

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

include/uapi/drm/drm_mode.h +6 -0

index aae71cb..c43648c 100644
@@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
 
 #define DRM_MODE_FB_INTERLACED	(1<<0) /* for interlaced framebuffers */
 
+/*
+ * Format Modifiers:
+ *
+ * Format modifiers describe, typically, a re-ordering or modification
+ * of the data in a plane of an FB.  This can be used to express tiled/
+ * swizzled formats, or compression, or a combination of the two.

include/uapi/drm/drm_mode.h +14 -0

+
+/* Modifier values: */
+/* 64x32 macroblock, ie NV12MT: */
+#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
+/* ... more */
+
 struct drm_mode_fb_cmd2 {
 	__u32 fb_id;
 	__u32 width, height;

@@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
 	 * So it would consist of Y as offsets[0] and UV as
 	 * offsets[1].  Note that offsets[0] will generally
 	 * be 0 (but this is not required).
+	 *
+	 * To accommodate tiled, compressed, etc formats, a per-plane
+	 * modifier can be specified.  The default value of zero
+	 * indicates "native" format as specified by the fourcc.
+	 * Vendor specific modifier token.  This allows, for example,
+	 * different tiling/swizzling pattern on different planes.
+	 * See discussion above of DRM_FOURCC_MOD_xxx.
 	 */
 	__u32 handles[4];
 	__u32 pitches[4]; /* pitch for each plane */
 	__u32 offsets[4]; /* offset of each plane */
+	__u32 modifier[4]; /* ie, tiling, compressed (per plane) */
 };
 
 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

Signature · 1 line

2.1.0

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Quoted 7 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

8bits should be enough, will take a while until we have more than 250 gpu drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits though to make sure that there's enough space in the bitfiel to encode substrides and stuff like that, in case anyone needs it. For vendor ids I'd just go with first come and starting at 1 (i.e. rename yours). That way we make it clear that until a patch is merged upstream the id isn't reserved yet. drm-next should be sufficient as registry imo.

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

But yeah I like this and in i915 we definitely need this. skl adds a bunch of framebuffer layouts where we need to spec tiling in more detail.

Overall I like.

Quoted 5 lines

Signed-off-by Rob Clark <[email protected]>

---

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

Quoted 27 lines · On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Quoted 7 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

8bits should be enough, will take a while until we have more than 250 gpu drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits though to make sure that there's enough space in the bitfiel to encode substrides and stuff like that, in case anyone needs it. For vendor ids I'd just go with first come and starting at 1 (i.e. rename yours). That way we make it clear that until a patch is merged upstream the id isn't reserved yet. drm-next should be sufficient as registry imo.

Quoted 5 lines

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

Could we please add the check in core code instead of drivers ?

Quoted 31 lines · Laurent Pinchart

Quoted 30 lines · On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Quoted 7 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

8bits should be enough, will take a while until we have more than 250 gpu drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits though to make sure that there's enough space in the bitfiel to encode substrides and stuff like that, in case anyone needs it. For vendor ids I'd just go with first come and starting at 1 (i.e. rename yours). That way we make it clear that until a patch is merged upstream the id isn't reserved yet. drm-next should be sufficient as registry imo.

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Quoted 5 lines

Quoted 3 lines

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

Could we please add the check in core code instead of drivers ?

Nope since then no driver could ever use that extension. Defeats the point ;-)

Hi Daniel,

Quoted 36 lines · On Monday 15 December 2014 08:33:10 Daniel Vetter wrote:

Quoted 35 lines · Laurent Pinchart

Quoted 33 lines · On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Quoted 7 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

8bits should be enough, will take a while until we have more than 250 gpu drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits though to make sure that there's enough space in the bitfiel to encode substrides and stuff like that, in case anyone needs it. For vendor ids I'd just go with first come and starting at 1 (i.e. rename yours). That way we make it clear that until a patch is merged upstream the id isn't reserved yet. drm-next should be sufficient as registry imo.

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

Quoted 4 lines

Quoted 1 line

Could we please add the check in core code instead of drivers ?

Nope since then no driver could ever use that extension. Defeats the point ;-)

Except if we follow the proposal of adding a flag to tell whether a driver supports the extension ;-)

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

Could we please add the check in core code instead of drivers ?

Nope since then no driver could ever use that extension. Defeats the point ;-)

Quoted 2 lines

Except if we follow the proposal of adding a flag to tell whether a driver supports the extension ;-)

I'm not a terrible big fan of driver flags, mostly because I've seen too much of the horrible stuff in dri1. Imo much better to pass everything to drivers and help them out with helpers if needed. I might be going overboard a bit with my bias against driver flags ;-)

Quoted 12 lines · Daniel Vetter

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet another case of an ioctl with fields that can't actually be used everywhere.

Could we please add the check in core code instead of drivers ?

Nope since then no driver could ever use that extension. Defeats the point ;-)

Quoted 7 lines

Quoted 2 lines

Except if we follow the proposal of adding a flag to tell whether a driver supports the extension ;-)

I'm not a terrible big fan of driver flags, mostly because I've seen too much of the horrible stuff in dri1. Imo much better to pass everything to drivers and help them out with helpers if needed. I might be going overboard a bit with my bias against driver flags ;-)

I ventured a little ways down the thought path of adding list of supported modifier tokens per plane.. and then doing more complete checks in the core. But then the question is, what about cases where some tiling format is only supported for UV but not Y, etc.. it quickly gets ugly.

I think for now better to let the driver do this (with a must_be_all_zeros() helper for what I expect will be the common case initially). If common patterns emerge, then we refactor out a better helper..

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Hey,

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Cool, thanks a lot for looking at this! My one comment is that we could maybe go even further: keep the current modifier as a strict pixel-layout modifier (e.g. tiled, compressed - anything that affects how you actually determine pixel location), and then support an extra (perhaps non-vendor-namespaced) argument for optional pixel _interpretation_ modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting to properly attack things like chroma siting, and being able to specify narrow/wide YUV range is pretty important for STB/DTV in particular. And they're actually starting to move to KMS, too ...

It might be useful to make the interpretation modifiers bitmaskable, so they can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting), but I can't think of a usecase for combining multiple layout modifiers (e.g. this tiling | that compression).

Quoted 8 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much easier for generic userspace.

Cheers,
Daniel

Quoted 15 lines · Daniel Stone

Hey,

Quoted 10 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Quoted 9 lines

Cool, thanks a lot for looking at this! My one comment is that we could maybe go even further: keep the current modifier as a strict pixel-layout modifier (e.g. tiled, compressed - anything that affects how you actually determine pixel location), and then support an extra (perhaps non-vendor-namespaced) argument for optional pixel _interpretation_ modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting to properly attack things like chroma siting, and being able to specify narrow/wide YUV range is pretty important for STB/DTV in particular. And they're actually starting to move to KMS, too ...

Up until now I had sort of thought of things like YUV range as plane properties which could be updated (potentially on flip as part of atomic ioctl). But they are also additional metadata about how to properly interpret the pixel data contained in the buffer.

I guess chroma siting and YUV range would at least be one value that applies across all the bos/planes of the fb, rather than per plane?

It does make me briefly think of just letting us set properties on fb objects :-P (but maybe that is a bit overkill)

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

Quoted 6 lines

It might be useful to make the interpretation modifiers bitmaskable, so they can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting), but I can't think of a usecase for combining multiple layout modifiers (e.g. this tiling | that compression).

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Quoted 10 lines

Quoted 8 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Quoted 3 lines

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much easier for generic userspace.

I've locally made a few tweaks (64b and move some stuff to drm_fourcc.h)..

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:

It does make me briefly think of just letting us set properties on fb

objects :-P (but maybe that is a bit overkill)

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Quoted 5 lines

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the fb and let pixel format and such as properties. That would be a fairly natural line IMO since it would be enough data to do a blit, but not enough to actually interpret the pixel data. But we already went beyond that with pixel formats. So I'm not sure how far we want to go.

Also all this chroma siting and colorspace stuff definitely runs into hardware specific limitations so having some way to tell userspace what is possible would be nice as you said. Properties seem a decent match for that.

Quoted 1 line

Quoted 1 line

It might be useful to make the interpretation modifiers bitmaskable,

so they

Quoted 1 line

Quoted 1 line

can be combined (e.g. wide-range/unclamped YUV | whatever chroma

siting),

Quoted 1 line

Quoted 1 line

but I can't think of a usecase for combining multiple layout modifiers

(e.g.

Quoted 10 lines

Quoted 1 line

this tiling | that compression).

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a

relatively

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much

easier

Quoted 3 lines

Quoted 1 line

for generic userspace.

I've locally made a few tweaks (64b and move some stuff to

drm_fourcc.h)..

Quoted 8 lines

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

Indeed the format specific limitations are problem. Properties can't handle that. We'd need to have some kind of caps for each plane+format combination if we want to deal with that. But I don't think we can still make it handle all the hw limitations, so I'm not sure it's worth going down this path.

Quoted 1 line · Rob Clark

On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <[email protected]>

Quoted 7 lines · wrote:

Quoted 7 lines

Hey,

Quoted 4 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver

specific

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to,

if

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Cool, thanks a lot for looking at this! My one comment is that we could maybe go even further: keep the current modifier as a strict

pixel-layout

Quoted 1 line

Quoted 1 line

modifier (e.g. tiled, compressed - anything that affects how you

actually

Quoted 3 lines

Quoted 3 lines

determine pixel location), and then support an extra (perhaps non-vendor-namespaced) argument for optional pixel _interpretation_ modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is

starting

Quoted 2 lines

Quoted 2 lines

to properly attack things like chroma siting, and being able to specify narrow/wide YUV range is pretty important for STB/DTV in particular.

And

Quoted 9 lines

Quoted 1 line

they're actually starting to move to KMS, too ...

Up until now I had sort of thought of things like YUV range as plane properties which could be updated (potentially on flip as part of atomic ioctl). But they are also additional metadata about how to properly interpret the pixel data contained in the buffer.

I guess chroma siting and YUV range would at least be one value that applies across all the bos/planes of the fb, rather than per plane?

There is the DV case where the chroma is sampled at different points for Cb and Cr. So we could in theory specify chroma siting per-plane. But it seems to me that it'd be enough to have it for the entire fb. I had some ideas posted years ago. Here's [1] one at least.

[1]
http://lists.freedesktop.org/archives/dri-devel/2011-November/016379.html

Quoted 3 lines

It does make me briefly think of just letting us set properties on fb objects :-P (but maybe that is a bit overkill)

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Quoted 6 lines

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the fb and let pixel format and such as properties. That would be a fairly natural line IMO since it would be enough data to do a blit, but not enough to actually interpret the pixel data. But we already went beyond that with pixel formats. So I'm not sure how far we want to go.

Also all this chroma siting and colorspace stuff definitely runs into hardware specific limitations so having some way to tell userspace what is possible would be nice as you said. Properties seem a decent match for that.

Quoted 2 lines

Quoted 2 lines

It might be useful to make the interpretation modifiers bitmaskable,

so they

Quoted 1 line

Quoted 1 line

can be combined (e.g. wide-range/unclamped YUV | whatever chroma

siting),

Quoted 1 line

Quoted 1 line

but I can't think of a usecase for combining multiple layout modifiers

(e.g.

Quoted 16 lines

Quoted 2 lines

this tiling | that compression).

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Quoted 4 lines

Quoted 4 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a

relatively

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much

easier

Quoted 3 lines

Quoted 1 line

for generic userspace.

I've locally made a few tweaks (64b and move some stuff to

drm_fourcc.h)..

Quoted 8 lines

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

Indeed the format specific limitations are problem. Properties can't handle that. We'd need to have some kind of caps for each plane+format combination if we want to deal with that. But I don't think we can still make it handle all the hw limitations, so I'm not sure it's worth going down this path.

--
Ville Syrjälä
Intel OTC

Hi,

Quoted 9 lines · Ville Syrjälä

On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:

It does make me briefly think of just letting us set properties on fb

objects :-P (but maybe that is a bit overkill)

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Mm, it does seem a bit weird. Yes, they are relative to how the plane interprets things, but then again so is the format surely. Not to mention another thing to go wrong, if someone forgets to set and/or clear it when changing the plane. Keeping it in the fb eliminates that possibility.

Quoted 12 lines

Quoted 5 lines

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the fb and let pixel format and such as properties. That would be a fairly natural line IMO since it would be enough data to do a blit, but not enough to actually interpret the pixel data. But we already went beyond that with pixel formats. So I'm not sure how far we want to go.

Quoted 4 lines

Also all this chroma siting and colorspace stuff definitely runs into hardware specific limitations so having some way to tell userspace what is possible would be nice as you said. Properties seem a decent match for that.

Yeah, that's a good idea actually, especially since different planes do have different capabilities.

Quoted 17 lines

Quoted 1 line

Quoted 1 line

It might be useful to make the interpretation modifiers bitmaskable,

so they

Quoted 1 line

Quoted 1 line

can be combined (e.g. wide-range/unclamped YUV | whatever chroma

siting),

Quoted 1 line

Quoted 1 line

but I can't think of a usecase for combining multiple layout modifiers

(e.g.

Quoted 10 lines

Quoted 1 line

this tiling | that compression).

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 28 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a

relatively

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much

easier

Quoted 3 lines

Quoted 1 line

for generic userspace.

I've locally made a few tweaks (64b and move some stuff to

drm_fourcc.h)..

Quoted 8 lines

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

Quoted 5 lines

Indeed the format specific limitations are problem. Properties can't handle that. We'd need to have some kind of caps for each plane+format combination if we want to deal with that. But I don't think we can still make it handle all the hw limitations, so I'm not sure it's worth going down this path.

Well, you don't have to solve literally everything at once. Just having a list of formats which could possibly be supported if you did the right thing, would be a hell of a lot better than punting to userspace, which either a) has to have hardware-specific knowledge in every component (compositor, media library, etc etc), or b) brute-force it. The lack of any format query in EGLImage dmabuf import is a serious, serious, serious, pain when trying to do generic userspace (e.g. compositor feeds GStreamer a list of formats which are supported by the hardware). I get that there are combinations that could fail, but that's true of everything. At least narrowing down the problem space a bit is an enormous help.

Cheers,
Daniel

Quoted 132 lines · Ville Syrjälä

Quoted 1 line · Rob Clark

On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <[email protected]>

Quoted 7 lines · wrote:

Quoted 7 lines

Hey,

Quoted 4 lines · Rob Clark

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver

specific

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to,

if

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Cool, thanks a lot for looking at this! My one comment is that we could maybe go even further: keep the current modifier as a strict

pixel-layout

Quoted 1 line

Quoted 1 line

modifier (e.g. tiled, compressed - anything that affects how you

actually

Quoted 3 lines

Quoted 3 lines

determine pixel location), and then support an extra (perhaps non-vendor-namespaced) argument for optional pixel _interpretation_ modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is

starting

Quoted 2 lines

Quoted 2 lines

to properly attack things like chroma siting, and being able to specify narrow/wide YUV range is pretty important for STB/DTV in particular.

And

Quoted 9 lines

Quoted 1 line

they're actually starting to move to KMS, too ...

Up until now I had sort of thought of things like YUV range as plane properties which could be updated (potentially on flip as part of atomic ioctl). But they are also additional metadata about how to properly interpret the pixel data contained in the buffer.

I guess chroma siting and YUV range would at least be one value that applies across all the bos/planes of the fb, rather than per plane?

There is the DV case where the chroma is sampled at different points for Cb and Cr. So we could in theory specify chroma siting per-plane. But it seems to me that it'd be enough to have it for the entire fb. I had some ideas posted years ago. Here's [1] one at least.

[1]
http://lists.freedesktop.org/archives/dri-devel/2011-November/016379.html

Quoted 3 lines

It does make me briefly think of just letting us set properties on fb objects :-P (but maybe that is a bit overkill)

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Quoted 6 lines

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the fb and let pixel format and such as properties. That would be a fairly natural line IMO since it would be enough data to do a blit, but not enough to actually interpret the pixel data. But we already went beyond that with pixel formats. So I'm not sure how far we want to go.

Also all this chroma siting and colorspace stuff definitely runs into hardware specific limitations so having some way to tell userspace what is possible would be nice as you said. Properties seem a decent match for that.

Quoted 2 lines

Quoted 2 lines

It might be useful to make the interpretation modifiers bitmaskable,

so they

Quoted 1 line

Quoted 1 line

can be combined (e.g. wide-range/unclamped YUV | whatever chroma

siting),

Quoted 1 line

Quoted 1 line

but I can't think of a usecase for combining multiple layout modifiers

(e.g.

Quoted 16 lines

Quoted 2 lines

this tiling | that compression).

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Quoted 4 lines

Quoted 4 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a

relatively

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Yeah, I'd second all of danvet's comments here, as well as adding a new ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much

easier

Quoted 3 lines

Quoted 1 line

for generic userspace.

I've locally made a few tweaks (64b and move some stuff to

drm_fourcc.h)..

Quoted 8 lines

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

Indeed the format specific limitations are problem. Properties can't handle that. We'd need to have some kind of caps for each plane+format combination if we want to deal with that. But I don't think we can still make it handle all the hw limitations, so I'm not sure it's worth going down this path.

--
Ville Syrjälä
Intel OTC

Quoted 7 lines

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 6 lines

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

another argument in favor of plane properties, I think. This way userspace can query what is actually possibly and we don't implicitly give userspace the idea that display hw can handle something that it doesn't..

Quoted 18 lines · Rob Clark

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 6 lines

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

Quoted 4 lines

another argument in favor of plane properties, I think. This way userspace can query what is actually possibly and we don't implicitly give userspace the idea that display hw can handle something that it doesn't..

Well, we don't have properties to describe a lot of the limitations. I'm not sure we want to add tons of read-only properties for that. And as stated, sometimes the limitations depend on other properties/pixel format/etc. so seems rather hard to describe in a sane way that would actually be useful to userspace.

One idea that came up again just yesterday would be to have the kernel assign the planes on behalf of userspace. But that would then mean we need some kind of virtual plane layer on top so that the virtual plane state gets tracked correctly, or userspace would need to pass in the entire state for every display update. Also soon it may start to look like we're implementing some kind of compositor in the kernel. Another other approach might be to implement this plane assignment stuff in libdrm and duplicate some hw specific knowledge there.

--
Ville Syrjälä
Intel OTC

Quoted 24 lines · Ville Syrjälä

Quoted 18 lines · Rob Clark

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 6 lines

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

Quoted 4 lines

another argument in favor of plane properties, I think. This way userspace can query what is actually possibly and we don't implicitly give userspace the idea that display hw can handle something that it doesn't..

Quoted 5 lines

Well, we don't have properties to describe a lot of the limitations. I'm not sure we want to add tons of read-only properties for that. And as stated, sometimes the limitations depend on other properties/pixel format/etc. so seems rather hard to describe in a sane way that would actually be useful to userspace.

sorry, wasn't quite what I meant.. What I meant was that YUV range and siting properties would probably be enum properties, so userspace could see which enum values are supported.

r/o props could be a way to deal w/ some limits. Other limits, it could just be a matter of expressing the correct range as we convert things to properties for atomic.

Quoted 8 lines

One idea that came up again just yesterday would be to have the kernel assign the planes on behalf of userspace. But that would then mean we need some kind of virtual plane layer on top so that the virtual plane state gets tracked correctly, or userspace would need to pass in the entire state for every display update. Also soon it may start to look like we're implementing some kind of compositor in the kernel. Another other approach might be to implement this plane assignment stuff in libdrm and duplicate some hw specific knowledge there.

I kinda lean towards userspace. I don't want to preclude the case of a smart userspace (which has some driver specific userspace piece).. could be an interesting idea to have something in libdrm.

Quoted 1 line · Daniel Stone

On 12 December 2014 at 14:56, Ville Syrjälä <

[email protected]>

Quoted 2 lines

wrote:
Having them separated is still kinda nice though, for the same rationale

as

Quoted 2 lines

the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going

to

Quoted 3 lines

be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

But for some simpler cases like Xv it would seem perfectly OK to use the less strict rules. Well, unless someone implements Xv in a way that can also transparently switch between display planes and GL/software rendering.

Quoted 5 lines

Well, you don't have to solve literally everything at once. Just having a list of formats which could possibly be supported if you did the right thing, would be a hell of a lot better than punting to userspace, which either a) has to have hardware-specific knowledge in every component (compositor, media library, etc etc), or b) brute-force it. The lack of

any

Quoted 1 line

format query in EGLImage dmabuf import is a serious, serious, serious,

pain

Quoted 1 line

when trying to do generic userspace (e.g. compositor feeds GStreamer a

list

Quoted 3 lines

of formats which are supported by the hardware). I get that there are combinations that could fail, but that's true of everything. At least narrowing down the problem space a bit is an enormous help.

We alredy have a list of supported formats. The problem is when specific formats impose additonal constraints (eg. more restricted scaling factor limits).

Hi,

Quoted 14 lines · Ville Syrjälä

Quoted 1 line · Daniel Stone

On 12 December 2014 at 14:56, Ville Syrjälä <

[email protected]>

Quoted 2 lines

wrote:
Having them separated is still kinda nice though, for the same rationale

as

Quoted 2 lines

the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going

to

Quoted 3 lines

be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 7 lines

Well, it will look nasty when switching between GL and display composition the GL path does the right thing an display path doesn't/ And we already have that problem with the fuzzy alignment/scaling restriction stuff. So I think we will want some kind of strict flag somewhere to allow the user to specify that they'd rather fail the whole thing and fall back to GL rather than annoy the user.

If you're doing it through GL, you've already lost. Either you're doing some magic behind the user's back to bind multi-planar dmabuf-EGLImages to TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES, which forces you to use linear/nearest filtering. Even if you do use TEXTURE_2D binding, the EGLImage import spec does exactly the same as what's suggested here, and treats them as hints, which the implementation can use or ignore. So far I don't know of any implementation which doesn't ignore them.

FWIW, i965 completely disallows multi-planar EGLImage imports in the first place. Mali as shipped on ChromeOS forces you to use TEXTURE_EXTERNAL_OES. Soooo ...

Quoted 3 lines

But for some simpler cases like Xv it would seem perfectly OK to use the less strict rules. Well, unless someone implements Xv in a way that can also transparently switch between display planes and GL/software rendering.

Well sure, if you absolutely want to ensure it works, you're going to need some kind of query. Maybe, if the range/chroma-siting ones were part of a bitmask, you could steal the top bit to mark that the hints are actually requirements, and to fail if you can't respect the hints.

Quoted 2 lines · Daniel Stone

If you're doing it through GL, you've already lost. Either you're doing some magic behind the user's back to bind multi-planar dmabuf-EGLImages

to

Quoted 5 lines

TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES, which forces you to use linear/nearest filtering. Even if you do use TEXTURE_2D binding, the EGLImage import spec does exactly the same as what's suggested here, and treats them as hints, which the implementation can use or ignore. So far I don't know of any implementation which

doesn't

Quoted 1 line

ignore them.

Well anyone who is serious about quality ought to handle that stuff. Or at least make sure both GL/whatever and planes ignore the hints in the same way. So if you GL implementation is lax then you anyway need to have some driver/hardware specific knowledge to know which way to go when using the plane path to get matching output.

Quoted 1 line

Quoted 1 line

But for some simpler cases like Xv it would seem perfectly OK to use

the

Quoted 2 lines

Quoted 2 lines

less strict rules. Well, unless someone implements Xv in a way that can also transparently switch between display planes and GL/software

rendering.

Quoted 1 line

Quoted 1 line

Well sure, if you absolutely want to ensure it works, you're going to

need

Quoted 3 lines

some kind of query. Maybe, if the range/chroma-siting ones were part of a bitmask, you could steal the top bit to mark that the hints are actually requirements, and to fail if you can't respect the hints.

I was more thinking of some global "I want exactly what I said" kind of knob. Maybe as a client cap type of thingy.

Hi,

Quoted 10 lines · Ville Syrjälä

Quoted 2 lines · Daniel Stone

If you're doing it through GL, you've already lost. Either you're doing some magic behind the user's back to bind multi-planar dmabuf-EGLImages

to

Quoted 5 lines

TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES, which forces you to use linear/nearest filtering. Even if you do use TEXTURE_2D binding, the EGLImage import spec does exactly the same as what's suggested here, and treats them as hints, which the implementation can use or ignore. So far I don't know of any implementation which

Quoted 9 lines

doesn't

Quoted 1 line

ignore them.

Well anyone who is serious about quality ought to handle that stuff. Or at least make sure both GL/whatever and planes ignore the hints in the same way. So if you GL implementation is lax then you anyway need to have some driver/hardware specific knowledge to know which way to go when using the plane path to get matching output.

Anyone who's serious about quality and is also using GL for video, is not serious about quality. Or accurate timing.

Quoted 14 lines

Quoted 3 lines

Quoted 3 lines

But for some simpler cases like Xv it would seem perfectly OK to use

the

Quoted 2 lines

Quoted 2 lines

less strict rules. Well, unless someone implements Xv in a way that can also transparently switch between display planes and GL/software

rendering.

Quoted 2 lines

Well sure, if you absolutely want to ensure it works, you're going to

need

Quoted 3 lines

some kind of query. Maybe, if the range/chroma-siting ones were part of a bitmask, you could steal the top bit to mark that the hints are actually requirements, and to fail if you can't respect the hints.

Quoted 3 lines

I was more thinking of some global "I want exactly what I said" kind of knob. Maybe as a client cap type of thingy.

I like the idea of keeping it local to the chroma-siting/range hints, because it makes it far more clear exactly what it affects.

Quoted 1 line · Daniel Stone

On 12 December 2014 at 18:00, Ville Syrjälä <

[email protected]>

Quoted 8 lines

Quoted 6 lines · wrote:

Well anyone who is serious about quality ought to handle that stuff. Or at least make sure both GL/whatever and planes ignore the hints in the same way. So if you GL implementation is lax then you anyway need to have some driver/hardware specific knowledge to know which way to go when using the plane path to get matching output.

Quoted 2 lines

Anyone who's serious about quality and is also using GL for video, is not serious about quality. Or accurate timing.

You're too hung up on the "GL" there. It doesn't actually matter what you use to render the video when not using the display hardware. The same problem remains.

Quoted 5 lines

Quoted 2 lines

I was more thinking of some global "I want exactly what I said" kind of knob. Maybe as a client cap type of thingy.

I like the idea of keeping it local to the chroma-siting/range hints, because it makes it far more clear exactly what it affects.

You're not thinking wide enough. We would need to add similar hints to pretty much every property.

Hi,

Quoted 12 lines · Ville Syrjälä

Quoted 1 line · Daniel Stone

On 12 December 2014 at 18:00, Ville Syrjälä <

[email protected]>

Quoted 8 lines

Quoted 6 lines · wrote:

Well anyone who is serious about quality ought to handle that stuff. Or at least make sure both GL/whatever and planes ignore the hints in the same way. So if you GL implementation is lax then you anyway need to have some driver/hardware specific knowledge to know which way to go when using the plane path to get matching output.

Quoted 6 lines

Quoted 2 lines

Anyone who's serious about quality and is also using GL for video, is not serious about quality. Or accurate timing.

You're too hung up on the "GL" there. It doesn't actually matter what you use to render the video when not using the display hardware. The same problem remains.

Yes, the problem being that sometimes you want to force the hardware to do exactly what you want and literally nothing else, and sometimes you don't care.

I posit that a hint-with-optional-force interface is best, because:
- if your hint isn't supported, what do you do? fall back to software?
- the number of people who care enough to force it is vanishingly small,
which is partly shown by how:
- it matches GL semantics
- not a great deal of hardware supports them

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I was more thinking of some global "I want exactly what I said" kind of knob. Maybe as a client cap type of thingy.

I like the idea of keeping it local to the chroma-siting/range hints, because it makes it far more clear exactly what it affects.

Quoted 2 lines

You're not thinking wide enough. We would need to add similar hints to pretty much every property.

Which other properties do we have right now that drivers treat as optional hints?

If the answer involves hypothetical new properties, what are they?

Hi,

Quoted 7 lines · Ville Syrjälä

Quoted 3 lines · Rob Clark

It does make me briefly think of just letting us set properties on fb

objects :-P (but maybe that is a bit overkill)

Quoted 2 lines

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Mm, it does seem a bit weird. Yes, they are relative to how the plane interprets things, but then again so is the format surely. Not to mention another thing to go wrong, if someone forgets to set and/or clear it when changing the plane. Keeping it in the fb eliminates that possibility.

Quoted 13 lines

Quoted 6 lines

I suppose the semi-custom plane property approach is a bit easier to extend-as-we-go, and we already have a mechanism so userspace can query about what is actually supported. But this does feel a bit more like attributes of the fb. I'm interested if anyone has particularly good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the fb and let pixel format and such as properties. That would be a fairly natural line IMO since it would be enough data to do a blit, but not enough to actually interpret the pixel data. But we already went beyond that with pixel formats. So I'm not sure how far we want to go.

Quoted 4 lines

Also all this chroma siting and colorspace stuff definitely runs into hardware specific limitations so having some way to tell userspace what is possible would be nice as you said. Properties seem a decent match for that.

Yeah, that's a good idea actually, especially since different planes do have different capabilities.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

It might be useful to make the interpretation modifiers bitmaskable, so they
can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting),
but I can't think of a usecase for combining multiple layout modifiers (e.g.
this tiling | that compression).

Quoted 8 lines

Quoted 8 lines

Yeah, I think the vendor-range part of the token, the vendor would probably want to define as a bitmask or set of bitfields so that they could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw generations in the future that scheme will break down.. so maybe a big switch of #define cases is better than trying to interpret the modifier token)

Having them separated is still kinda nice though, for the same rationale as the EGLImage import extension having them as hints. If your hardware doesn't support the tiling/compression format you use, then you're going to be showing absolute garbage. But if it doesn't support your exact chroma-siting or YUV range request, it'll still be totally viewable, just not entirely perfect. So I don't see the logic in failing these.

Quoted 28 lines

Quoted 27 lines

Quoted 11 lines

Quoted 9 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively
arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

Quoted 5 lines

Yeah, I'd second all of danvet's comments here, as well as adding a new
ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much easier
for generic userspace.

I've locally made a few tweaks (64b and move some stuff to drm_fourcc.h)..

I was kicking around the idea of letting plane specify an array of supported format modifiers, and adding this to getplane ioctl, as an alternative to a cap. That plus wiring up some checking to disallow addfb2 for a format + modifiers not supported by at least one plane. Although some hw could only support certain tiling patterns for certain layers of an fb (ie. luma vs chroma). So I may scrap that idea and just go back to cap.

Quoted 5 lines

Indeed the format specific limitations are problem. Properties can't handle that. We'd need to have some kind of caps for each plane+format combination if we want to deal with that. But I don't think we can still make it handle all the hw limitations, so I'm not sure it's worth going down this path.

Well, you don't have to solve literally everything at once. Just having a list of formats which could possibly be supported if you did the right thing, would be a hell of a lot better than punting to userspace, which either a) has to have hardware-specific knowledge in every component (compositor, media library, etc etc), or b) brute-force it. The lack of any format query in EGLImage dmabuf import is a serious, serious, serious, pain when trying to do generic userspace (e.g. compositor feeds GStreamer a list of formats which are supported by the hardware). I get that there are combinations that could fail, but that's true of everything. At least narrowing down the problem space a bit is an enormous help.

Cheers,
Daniel

Quoted 11 lines · Daniel Stone

Hi,

Quoted 7 lines · Ville Syrjälä

Quoted 3 lines · Rob Clark

It does make me briefly think of just letting us set properties on fb

objects :-P (but maybe that is a bit overkill)

Quoted 9 lines

Quoted 2 lines

Yeah I had the same idea at some point. But then I decided that we could just have these as properties on the plane.

Mm, it does seem a bit weird. Yes, they are relative to how the plane interprets things, but then again so is the format surely. Not to mention another thing to go wrong, if someone forgets to set and/or clear it when changing the plane. Keeping it in the fb eliminates that possibility.

yeah.. logically it seems nicer for them to be prop's on fb's. The drawback is having to invent some bit of infrastructure to support that. Avoidance of inheriting someone else's plane prop's might be enough justification to invent that infrastructure. But fb prop's don't really help w/ the whole not-all-planes-are-the-same thing..

On 10 December 2014 at 17:17, Rob Clark <[email protected] <mailto:[email protected]>> wrote:

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

Cool, thanks a lot for looking at this! My one comment is that we could maybe go even further: keep the current modifier as a strict pixel-layout modifier (e.g. tiled, compressed - anything that affects how you actually determine pixel location), and then support an extra (perhaps non-vendor-namespaced) argument for optional pixel _interpretation_ modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting to properly attack things like chroma siting, and being able to specify narrow/wide YUV range is pretty important for STB/DTV in particular. And they're actually starting to move to KMS, too ...

It might be useful to make the interpretation modifiers bitmaskable, so they can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting), but I can't think of a usecase for combining multiple layout modifiers (e.g. this tiling | that compression).

Quoted 4 lines · Daniel Stone

It might be useful to make the interpretation modifiers bitmaskable, so they can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting), but I can't think of a usecase for combining multiple layout modifiers (e.g. this tiling | that compression).

I might be misunderstanding what you're referring to, but FWIW: With AMD GPUs, the compression format and tiling parameters can be chosen (mostly) independently.

Hi,

Quoted 10 lines · Michel Dänzer

Quoted 4 lines · Daniel Stone

It might be useful to make the interpretation modifiers bitmaskable, so they can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting), but I can't think of a usecase for combining multiple layout modifiers (e.g. this tiling | that compression).

I might be misunderstanding what you're referring to, but FWIW: With AMD GPUs, the compression format and tiling parameters can be chosen (mostly) independently.

Should've been a little more clear. Definitely each vendor will have their own combination of modes, but I mostly meant that I don't really see a use for VENDOR_AMD(compression_1) | VENDOR_OTHER(tiling_N), or VENDOR_ARM(afbc) | VENDOR_SAMSUNG(tiling_12x64). Each vendor's space would be free to use the 24 sub-bits as a bitmask, but I don't think there's a need for combining tokens from multiple vendor spaces.

Cheers,
Daniel

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

TODO move definition of tokens to drm_fourcc.h?

Signed-off-by Rob Clark <[email protected]>

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

include/uapi/drm/drm_mode.h +22 -0

index aae71cb..c43648c 100644
@@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
 
 #define DRM_MODE_FB_INTERLACED	(1<<0) /* for interlaced framebuffers */
 
+/*
+ * Format Modifiers:
+ *
+ * Format modifiers describe, typically, a re-ordering or modification
+ * of the data in a plane of an FB.  This can be used to express tiled/
+ * swizzled formats, or compression, or a combination of the two.
+ *
+ * The upper 8 bits of the format modifier are a vendor-id as assigned
+ * below.  The lower 24 bits are assigned as vendor sees fit.
+ */
+
+/* Vendor Ids: */
+#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
+/* ... more */
+
+#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) <<
 24) | val)
+
+/* Modifier values: */
+/* 64x32 macroblock, ie NV12MT: */
+#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
+/* ... more */
+
 struct drm_mode_fb_cmd2 {
 	__u32 fb_id;

include/uapi/drm/drm_mode.h +8 -0

 	__u32 width, height;

@@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
 	 * So it would consist of Y as offsets[0] and UV as
 	 * offsets[1].  Note that offsets[0] will generally
 	 * be 0 (but this is not required).
+	 *
+	 * To accommodate tiled, compressed, etc formats, a per-plane
+	 * modifier can be specified.  The default value of zero
+	 * indicates "native" format as specified by the fourcc.
+	 * Vendor specific modifier token.  This allows, for example,
+	 * different tiling/swizzling pattern on different planes.
+	 * See discussion above of DRM_FOURCC_MOD_xxx.
 	 */
 	__u32 handles[4];
 	__u32 pitches[4]; /* pitch for each plane */
 	__u32 offsets[4]; /* offset of each plane */
+	__u32 modifier[4]; /* ie, tiling, compressed (per plane) */
 };
 
 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

Hi Rob,

Quoted 5 lines · On Wednesday 10 December 2014 12:17:51 Rob Clark wrote:

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

Quoted 4 lines

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

But it will change the size of the structure, and thus the ioctl value. You can't extend existing structures used in ioctls I'm afraid.

By the way, is thus calls for an addfb3, I would add reserved fields at the end of the structure to make future extensions possible without a new ioctl.

Quoted 67 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

TODO move definition of tokens to drm_fourcc.h?

Signed-off-by Rob Clark <[email protected]>

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

include/uapi/drm/drm_mode.h +22 -0

index aae71cb..c43648c 100644
@@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
 
 #define DRM_MODE_FB_INTERLACED       (1<<0) /* for interlaced framebuffers */
 
+/*
+ * Format Modifiers:
+ *
+ * Format modifiers describe, typically, a re-ordering or modification
+ * of the data in a plane of an FB.  This can be used to express tiled/
+ * swizzled formats, or compression, or a combination of the two.
+ *
+ * The upper 8 bits of the format modifier are a vendor-id as assigned
+ * below.  The lower 24 bits are assigned as vendor sees fit.
+ */
+
+/* Vendor Ids: */
+#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
+/* ... more */
+
+#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) <<
 24) | val)
+
+/* Modifier values: */
+/* 64x32 macroblock, ie NV12MT: */
+#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
+/* ... more */
+
 struct drm_mode_fb_cmd2 {
      __u32 fb_id;

      __u32 width, height;

include/uapi/drm/drm_mode.h +8 -0

@@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
       * So it would consist of Y as offsets[0] and UV as
       * offsets[1].  Note that offsets[0] will generally
       * be 0 (but this is not required).
+      *
+      * To accommodate tiled, compressed, etc formats, a per-plane
+      * modifier can be specified.  The default value of zero
+      * indicates "native" format as specified by the fourcc.
+      * Vendor specific modifier token.  This allows, for example,
+      * different tiling/swizzling pattern on different planes.
+      * See discussion above of DRM_FOURCC_MOD_xxx.
       */
      __u32 handles[4];
      __u32 pitches[4]; /* pitch for each plane */
      __u32 offsets[4]; /* offset of each plane */
+     __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
 };
 

include/uapi/drm/drm_mode.h struct drm_mode_fb_cmd2 { · line 388

Quoted 1 line

include/uapi/drm/drm_mode.h +0 -0

 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

--
Regards,

Laurent Pinchart

Quoted 8 lines · Laurent Pinchart

Hi Rob,

Quoted 5 lines · On Wednesday 10 December 2014 12:17:51 Rob Clark wrote:

In DRM/KMS we are lacking a good way to deal with tiled/compressed formats. Especially in the case of dmabuf/prime buffer sharing, where we cannot always rely on under-the-hood flags passed to driver specific gem-create ioctl to pass around these extra flags.

Quoted 7 lines

Quoted 4 lines

The proposal is to add a per-plane format modifier. This allows to, if necessary, use different tiling patters for sub-sampled planes, etc. The format modifiers are added at the end of the ioctl struct, so for legacy userspace it will be zero padded.

But it will change the size of the structure, and thus the ioctl value. You can't extend existing structures used in ioctls I'm afraid.

Actually, that is why it will work. Old userspace passes smaller size, drm_ioctl() zero pads the difference..

The issue is (potentially) in the other direction (new userspace, old kernel) since the old kernel will ignore the new fields. But that can be sorted w/ a cap/query

BR,
-R

Quoted 70 lines

By the way, is thus calls for an addfb3, I would add reserved fields at the end of the structure to make future extensions possible without a new ioctl.

Quoted 67 lines

TODO how best to deal with assignment of modifier token values? The rough idea was to namespace things with an 8bit vendor-id, and then beyond that it is treated as an opaque value. But that was a relatively arbitrary choice. There are cases where same tiling pattern and/or compression is supported by various different vendors. So we should standardize to use the vendor-id and value of the first one who documents the format?

TODO move definition of tokens to drm_fourcc.h?

Signed-off-by Rob Clark <[email protected]>

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

include/uapi/drm/drm_mode.h +22 -0

index aae71cb..c43648c 100644
@@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
 
 #define DRM_MODE_FB_INTERLACED       (1<<0) /* for interlaced framebuffers */
 
+/*
+ * Format Modifiers:
+ *
+ * Format modifiers describe, typically, a re-ordering or modification
+ * of the data in a plane of an FB.  This can be used to express tiled/
+ * swizzled formats, or compression, or a combination of the two.
+ *
+ * The upper 8 bits of the format modifier are a vendor-id as assigned
+ * below.  The lower 24 bits are assigned as vendor sees fit.
+ */
+
+/* Vendor Ids: */
+#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
+/* ... more */
+
+#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) <<
 24) | val)
+
+/* Modifier values: */
+/* 64x32 macroblock, ie NV12MT: */
+#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
+/* ... more */
+
 struct drm_mode_fb_cmd2 {
      __u32 fb_id;

      __u32 width, height;

include/uapi/drm/drm_mode.h +8 -0

@@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
       * So it would consist of Y as offsets[0] and UV as
       * offsets[1].  Note that offsets[0] will generally
       * be 0 (but this is not required).
+      *
+      * To accommodate tiled, compressed, etc formats, a per-plane
+      * modifier can be specified.  The default value of zero
+      * indicates "native" format as specified by the fourcc.
+      * Vendor specific modifier token.  This allows, for example,
+      * different tiling/swizzling pattern on different planes.
+      * See discussion above of DRM_FOURCC_MOD_xxx.
       */
      __u32 handles[4];
      __u32 pitches[4]; /* pitch for each plane */
      __u32 offsets[4]; /* offset of each plane */
+     __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
 };
 

Quoted 7 lines

include/uapi/drm/drm_mode.h struct drm_mode_fb_cmd2 { · line 388

Quoted 1 line

include/uapi/drm/drm_mode.h +0 -0

 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

--
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel