| Message ID | 1418231871-31088-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 10, 2014 at 12:17:51PM -0500, 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. > > 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? 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. > 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. > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index aae71cb..c43648c 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -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) Maybe a mask for the lower 24 bits? -Daniel > + > +/* 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 > -- > 2.1.0 >
Hey, On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com> 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). > 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
On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <daniel@fooishbar.org> wrote: > Hey, > > On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com> 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 ... 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. > 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) >> >> 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. 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. BR, -R > > Cheers, > Daniel
On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote: > On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <daniel@fooishbar.org> wrote: > > Hey, > > > > On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com> 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 ... > > 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 > > 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. > > 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. > > > 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) > > >> > >> 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. > > 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. 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.
Hi, On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > 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. > > 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. Yeah, that's a good idea actually, especially since different planes do have different capabilities. > > > 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) > 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. > > >> 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. > > > > 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. > > 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 On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote: > > On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <daniel@fooishbar.org> > wrote: > > > Hey, > > > > > > On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com> 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 ... > > > > 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 > > > > > 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. > > > > > 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. > > > > > > 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) > > > > >> > > >> 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. > > > > 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. > > 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 >
On Fri, Dec 12, 2014 at 03:11:02PM +0000, Daniel Stone wrote: > Hi, > > On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: > > > > 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. > > > > > 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. > > > Yeah, that's a good idea actually, especially since different planes do > have different capabilities. > > > > > > 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) > > > > 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. 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. > > > > > >> 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. > > > > > > 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. > > > > 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. We alredy have a list of supported formats. The problem is when specific formats impose additonal constraints (eg. more restricted scaling factor limits). > > Cheers, > Daniel > > On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: > > > > On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote: > > > On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone <daniel@fooishbar.org> > > wrote: > > > > Hey, > > > > > > > > On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com> 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 ... > > > > > > 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 > > > > > > > > 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. > > > > > > > > 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. > > > > > > > > > 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) > > > > > > >> > > > >> 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. > > > > > > 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. > > > > 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 > >
On Fri, Dec 12, 2014 at 10:11 AM, Daniel Stone <daniel@fooishbar.org> wrote: > Hi, > > On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: >> >> 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. > 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.. >> >> > 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. > > > Yeah, that's a good idea actually, especially since different planes do have > different capabilities. > >> >> > > 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) > > > 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. oh, sorry, I was just referring to the 'modifier token' stuff.. chroma-siting and YUV range are common enough that I think they should be something separate from the per-plane 'modifer token' >> >> > >> 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. >> > >> > 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. >> >> 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. > Yeah, I guess in the end the driver will still have to do some of it's own checks, the core can't do everything.. > > 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. > For sure, once we get the kms bit sorted we're going to want to take another pass at the eglimg extension and try to spiff it up a bit better. (and then I guess some day teach gallium / mesa-st about multi-planer external eglImages..) > Cheers, > Daniel >
On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> 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. 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.. BR, -R
On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote: > On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > >> > >> 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. > > > 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.
On Fri, Dec 12, 2014 at 11:14 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote: >> On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> >> >> >> 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. >> >> >> 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. 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. > 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. BR, -R > -- > Ville Syrjälä > Intel OTC
Hi, On 12 December 2014 at 15:30, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Dec 12, 2014 at 03:11:02PM +0000, Daniel Stone wrote: > > On 12 December 2014 at 14:56, Ville Syrjälä < > ville.syrjala@linux.intel.com> > > wrote: > > 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. > 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 ... > 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. > > 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. > > We alredy have a list of supported formats. The problem is when specific > formats impose additonal constraints (eg. more restricted scaling factor > limits). Where's the list of supported formats in this proposal? It just adds a modifier: there's no way to determine which modifiers are supported by a specific plane, which is what I really need to know. Right now, the only way is just brute-forcing your way through every single combination until you find one which succeeds. Like I said, I completely get that there are going to be specific/weird/arbitrary restrictions. There already are, such as scaling factors, maximum one non-primary plane per scanline, global bandwidth limits, etc etc. Those are not something KMS has ever attempted to solve, and I'm not suggesting that the modifier mechanism attempts to solve them. Cheers, Daniel
On Fri, Dec 12, 2014 at 05:11:50PM +0000, Daniel Stone wrote: > Hi, > > On 12 December 2014 at 15:30, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: > > > > On Fri, Dec 12, 2014 at 03:11:02PM +0000, Daniel Stone wrote: > > > On 12 December 2014 at 14:56, Ville Syrjälä < > > ville.syrjala@linux.intel.com> > > > wrote: > > > 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. > > > > 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. 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. > > 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 ... Without the strict flag you probably need to patch the kernel too then to make sure the planes ignore the hint the same way as your GL implementation. Or vice versa. > > 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. I was more thinking of some global "I want exactly what I said" kind of knob. Maybe as a client cap type of thingy. Another idea I had was to have such a flag in the atomic ioctl. But it seems impossible to handle that in a sane way unless you require the caller to specify the entire state every time the ioctl is called with the flag set.
Hi, On 12 December 2014 at 18:00, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Dec 12, 2014 at 05:11:50PM +0000, Daniel Stone wrote: > > 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. > > 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. > > > 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. > > 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. Cheers, Daniel
On Fri, Dec 12, 2014 at 06:05:49PM +0000, Daniel Stone wrote: > Hi, > > On 12 December 2014 at 18:00, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: > > > > On Fri, Dec 12, 2014 at 05:11:50PM +0000, Daniel Stone wrote: > > > 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. > > > > 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. 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. > > > > > > 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. > > > > 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.
On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote: > On Wed, Dec 10, 2014 at 12:17:51PM -0500, 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. > > > > 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? > > 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. > > > 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 ? > 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. > > > Signed-off-by: Rob Clark <robdclark@gmail.com> > > --- > > > > include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+)
Hi Rob, 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. > > 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. > 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 <robdclark@gmail.com> > --- > include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index aae71cb..c43648c 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -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; > @@ -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
On Fri, Dec 12, 2014 at 3:57 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > 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. >> >> 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 > 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. > >> 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 <robdclark@gmail.com> >> --- >> include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >> index aae71cb..c43648c 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -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; >> @@ -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 > > -- > Regards, > > Laurent Pinchart >
On Fri, Dec 12, 2014 at 04:33:38PM -0500, Rob Clark wrote: > On Fri, Dec 12, 2014 at 3:57 PM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > Hi Rob, > > > > 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. > >> > >> 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 Or by using up one flag in the ioctl to specify whether the new fields are valid or not. > > BR, > -R > > > > 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. > > > >> 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 <robdclark@gmail.com> > >> --- > >> include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > >> index aae71cb..c43648c 100644 > >> --- a/include/uapi/drm/drm_mode.h > >> +++ b/include/uapi/drm/drm_mode.h > >> @@ -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; > >> @@ -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 > > > > -- > > Regards, > > > > Laurent Pinchart > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote: > On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote: > > On Wed, Dec 10, 2014 at 12:17:51PM -0500, 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. > > > > > > 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? > > > > 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. > > > > > 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 ;-) Cheers, Daniel
On Fri, Dec 12, 2014 at 10:56:21AM -0500, Rob Clark wrote: > On Fri, Dec 12, 2014 at 10:11 AM, Daniel Stone <daniel@fooishbar.org> wrote: > > Hi, > > > > On 12 December 2014 at 14:56, Ville Syrjälä <ville.syrjala@linux.intel.com> > > wrote: > >> > >> 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. > > > > 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.. The nice thing with fbs currently is that all the metadata is invariant. Imo we should keep that for any prop extension, since it massively simplifies things for everyone. But I'm not sure that's so important since we already have (and probably always will have) a mess between plane and fb props. E.g. the rotation thing actually affects the pte ordering on skl. So would be nice to have as an invariant fb prop, but since it's now on the plane we can't change it. -Daniel
On Fri, Dec 12, 2014 at 12:05:41PM -0500, Rob Clark wrote: > On Fri, Dec 12, 2014 at 11:14 AM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote: > >> On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> >> > >> >> 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. > >> > >> > >> 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. > > 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. There's still the problem that yuv setting might not work on all formats, maybe it only works on the planar ones. > > 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. I expect this to happen sooner or later, probably a descendant of hwc (with all the assumtpions about single-crtc fixed and maybe some provisions to allow flips faster than vblanks). -Daniel
Hi, On 12 December 2014 at 18:22, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Dec 12, 2014 at 06:05:49PM +0000, Daniel Stone wrote: > > On 12 December 2014 at 18:00, Ville Syrjälä < > ville.syrjala@linux.intel.com> > > 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. > > > > > > > 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 > > > 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. Which other properties do we have right now that drivers treat as optional hints? If the answer involves hypothetical new properties, what are they? Cheers, Daniel
On 12.12.2014 20:27, Daniel Stone wrote: > On 10 December 2014 at 17:17, Rob Clark <robdclark@gmail.com > <mailto:robdclark@gmail.com>> 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). 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, On 16 December 2014 at 03:56, Michel Dänzer <michel@daenzer.net> wrote: > > On 12.12.2014 20:27, Daniel Stone wrote: > > 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
On Tue, Dec 16, 2014 at 3:01 AM, Daniel Stone <daniel@fooishbar.org> wrote: > Hi, > > On 16 December 2014 at 03:56, Michel Dänzer <michel@daenzer.net> wrote: >> >> On 12.12.2014 20:27, Daniel Stone wrote: >> > 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. > fwiw, I bumped it up to 64b (but haven't resent yet).. so now there are 56bits to divide as vendor sees fit between tiling/compression.. 56bits ought to be enough for anyone, right? :-P BR, -R > Cheers, > Daniel
On Mon, Dec 15, 2014 at 10:19:47PM +0000, Daniel Stone wrote: > Hi, > > On 12 December 2014 at 18:22, Ville Syrjälä <ville.syrjala@linux.intel.com> > wrote: > > > > On Fri, Dec 12, 2014 at 06:05:49PM +0000, Daniel Stone wrote: > > > On 12 December 2014 at 18:00, Ville Syrjälä < > > ville.syrjala@linux.intel.com> > > > 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. > > > > > > > > > > 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 > > > > > > 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. > > > Which other properties do we have right now that drivers treat as optional > hints? All the src/dst coordinates for one. > If the answer involves hypothetical new properties, what are they? Future ones might involve various color adjustment controls, at least if we want to attempt to make them hardware agnostic. Color keying properties might be have the same problem. I realize that treating color keying properties as hints doesn't really make much sense, but I'm not sure we have any sane alternative since the specific values you need to plug into the color keying register is rather hardware specific, and eg. with dst color keying it often depends on the pixel format of some other plane.
Hi Daniel, On Monday 15 December 2014 08:33:10 Daniel Vetter wrote: > On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote: > > On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote: > > > On Wed, Dec 10, 2014 at 12:17:51PM -0500, 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. > > > > > > > > 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? > > > > > > 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. > > > > > > > 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 > ;-) Except if we follow the proposal of adding a flag to tell whether a driver supports the extension ;-)
On Thu, Dec 18, 2014 at 10:54:14PM +0200, Laurent Pinchart wrote: > Hi Daniel, > > On Monday 15 December 2014 08:33:10 Daniel Vetter wrote: > > On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote: > > > On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote: > > > > On Wed, Dec 10, 2014 at 12:17:51PM -0500, 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. > > > > > > > > > > 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? > > > > > > > > 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. > > > > > > > > > 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 > > ;-) > > 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 ;-) -Daniel
On Thu, Dec 18, 2014 at 4:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > > > > 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 >> > ;-) >> >> 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.. BR, -R
Hi Rob, On Thursday 18 December 2014 19:55:24 Rob Clark wrote: > On Thu, Dec 18, 2014 at 4:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>>>>> 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 ;-) > >> > >> 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. From my experience with V4L2 expressing such constraints in a way that would be both simple and comprehensive isn't possible. We should aim for the common case, and I agree that finding out what the common case is would require implementing the feature first. However, it would be pretty easy to flag whether a driver supports this new API at all. That could be used to zero the extra fields. > 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..
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index aae71cb..c43648c 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -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; @@ -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
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 <robdclark@gmail.com> --- include/uapi/drm/drm_mode.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)