| Message ID | 1422466672-15833-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/28/2015 05:37 PM, Daniel Vetter wrote: > From: Rob Clark <robdclark@gmail.com> > > 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? Maybe: __u64 modifier[4]; __u64 vendor_modifier[4]; ? Regards, Tvrtko
On Wed, Jan 28, 2015 at 12:37 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > From: Rob Clark <robdclark@gmail.com> > > 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? iirc, I think we decided that drm_fourcc.h in drm-next is a sufficient authority for coordinating assignment of modifier token values, so we could probably drop this TODO. Perhaps it wouldn't hurt to document somewhere that, as with fourcc/format values, new additions are expected to come with some description of the format? > > v1: original > v1.5: increase modifier to 64b > > v2: Incorporate review comments from the big thread, plus a few more. > > - Add a getcap so that userspace doesn't have to jump through hoops. > - Allow modifiers only when a flag is set. That way drivers know when > they're dealing with old userspace and need to fish out e.g. tiling > from other information. > - After rolling out checks for ->modifier to all drivers I've decided > that this is way too fragile and needs an explicit opt-in flag. So > do that instead. > - Add a define (just for documentation really) for the "NONE" > modifier. Imo we don't need to add mask #defines since drivers > really should only do exact matches against values defined with > fourcc_mod_code. > - Drop the Samsung tiling modifier on Rob's request since he's not yet > sure whether that one is accurate. > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Daniel Stone <daniel@fooishbar.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Michel Dänzer <michel@daenzer.net> > Signed-off-by: Rob Clark <robdclark@gmail.com> (v1.5) > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > you forgot to change subject line to (v2).. but other than that, looks good Reviewed-by: Rob Clark <robdclark@gmail.com> > -- > > I've picked this up since we want to push out some fancy new tiling > modes soonish. No defines yet, but Tvrkto is working on the i915 parts > of this. > > I think the only part I haven't done from the discussion is exposing a > list of supported modifiers. Not sure that's really useful though > since all this is highly hw specific. > > And a note to driver writes: They need to check or the flag and in its > absence make a reasonable choice about the internal layet (e.g. for > i915 consult the tiling mode of the underlying bo). > -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++- > drivers/gpu/drm/drm_ioctl.c | 3 +++ > include/drm/drm_crtc.h | 3 +++ > include/uapi/drm/drm.h | 1 + > include/uapi/drm/drm_fourcc.h | 26 ++++++++++++++++++++++++++ > include/uapi/drm/drm_mode.h | 9 +++++++++ > 6 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 419f9d915c78..8090e3d64f67 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) > DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); > return -EINVAL; > } > + > + if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) { > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n", > + r->modifier[i], i); > + return -EINVAL; > + } > } > > return 0; > @@ -3337,7 +3343,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, > struct drm_framebuffer *fb; > int ret; > > - if (r->flags & ~DRM_MODE_FB_INTERLACED) { > + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { > DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); > return ERR_PTR(-EINVAL); > } > @@ -3353,6 +3359,12 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, > return ERR_PTR(-EINVAL); > } > > + if (r->flags & DRM_MODE_FB_MODIFIERS && > + !dev->mode_config.allow_fb_modifiers) { > + DRM_DEBUG_KMS("driver does not support fb modifiers\n"); > + return ERR_PTR(-EINVAL); > + } > + > ret = framebuffer_check(r); > if (ret) > return ERR_PTR(ret); > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 3785d66721f2..a6d773a61c2d 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > else > req->value = 64; > break; > + case DRM_CAP_ADDFB2_MODIFIERS: > + req->value = dev->mode_config.allow_fb_modifiers; > + break; > default: > return -EINVAL; > } > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 019c9b562144..767f7d4cab63 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1153,6 +1153,9 @@ struct drm_mode_config { > /* whether async page flip is supported or not */ > bool async_page_flip; > > + /* whether the driver supports fb modifiers */ > + bool allow_fb_modifiers; > + > /* cursor size */ > uint32_t cursor_width, cursor_height; > }; > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 01b2d6d0e355..ff6ef62d084b 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -630,6 +630,7 @@ struct drm_gem_open { > */ > #define DRM_CAP_CURSOR_WIDTH 0x8 > #define DRM_CAP_CURSOR_HEIGHT 0x9 > +#define DRM_CAP_ADDFB2_MODIFIERS 0x10 > > /** DRM_IOCTL_GET_CAP ioctl argument type */ > struct drm_get_cap { > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 646ae5f39f42..bf99557f1624 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -132,4 +132,30 @@ > #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */ > #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */ > > + > +/* > + * 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 56 bits are assigned as vendor sees fit. > + */ > + > +/* Vendor Ids: */ > +#define DRM_FORMAT_MOD_NONE 0 > +#define DRM_FORMAT_MOD_VENDOR_INTEL 0x01 > +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02 > +#define DRM_FORMAT_MOD_VENDOR_NV 0x03 > +#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04 > +#define DRM_FORMAT_MOD_VENDOR_QCOM 0x05 > +/* add more to the end as needed */ > + > +#define fourcc_mod_code(vendor, val) \ > + ((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL) > + > +/* Format Modifier tokens: */ > + > #endif /* DRM_FOURCC_H */ > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index ca788e01dab2..dbeba949462a 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -336,6 +336,7 @@ struct drm_mode_fb_cmd { > }; > > #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ > +#define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ > > struct drm_mode_fb_cmd2 { > __u32 fb_id; > @@ -356,10 +357,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_FORMAT_MOD_xxx. > */ > __u32 handles[4]; > __u32 pitches[4]; /* pitch for each plane */ > __u32 offsets[4]; /* offset of each plane */ > + __u64 modifier[4]; /* ie, tiling, compressed (per plane) */ > }; > > #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01 > -- > 2.1.4 >
On Wed, Jan 28, 2015 at 01:46:53PM -0500, Rob Clark wrote: > On Wed, Jan 28, 2015 at 12:37 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > From: Rob Clark <robdclark@gmail.com> > > > > 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? > > iirc, I think we decided that drm_fourcc.h in drm-next is a sufficient > authority for coordinating assignment of modifier token values, so we > could probably drop this TODO. Perhaps it wouldn't hurt to document > somewhere that, as with fourcc/format values, new additions are > expected to come with some description of the format? Oh right forgotten to drop the TODO. I'll add a comment to the header file. > > > > > v1: original > > v1.5: increase modifier to 64b > > > > v2: Incorporate review comments from the big thread, plus a few more. > > > > - Add a getcap so that userspace doesn't have to jump through hoops. > > - Allow modifiers only when a flag is set. That way drivers know when > > they're dealing with old userspace and need to fish out e.g. tiling > > from other information. > > - After rolling out checks for ->modifier to all drivers I've decided > > that this is way too fragile and needs an explicit opt-in flag. So > > do that instead. > > - Add a define (just for documentation really) for the "NONE" > > modifier. Imo we don't need to add mask #defines since drivers > > really should only do exact matches against values defined with > > fourcc_mod_code. > > - Drop the Samsung tiling modifier on Rob's request since he's not yet > > sure whether that one is accurate. > > > > Cc: Rob Clark <robdclark@gmail.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Daniel Stone <daniel@fooishbar.org> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Michel Dänzer <michel@daenzer.net> > > Signed-off-by: Rob Clark <robdclark@gmail.com> (v1.5) > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > you forgot to change subject line to (v2).. but other than that, looks good Ah, I generally don't keep a patch revision in the subject and forgot to update it ;-) > > Reviewed-by: Rob Clark <robdclark@gmail.com> > > > -- > > > > I've picked this up since we want to push out some fancy new tiling > > modes soonish. No defines yet, but Tvrkto is working on the i915 parts > > of this. > > > > I think the only part I haven't done from the discussion is exposing a > > list of supported modifiers. Not sure that's really useful though > > since all this is highly hw specific. > > > > And a note to driver writes: They need to check or the flag and in its > > absence make a reasonable choice about the internal layet (e.g. for > > i915 consult the tiling mode of the underlying bo). > > -Daniel > > --- > > drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++- > > drivers/gpu/drm/drm_ioctl.c | 3 +++ > > include/drm/drm_crtc.h | 3 +++ > > include/uapi/drm/drm.h | 1 + > > include/uapi/drm/drm_fourcc.h | 26 ++++++++++++++++++++++++++ > > include/uapi/drm/drm_mode.h | 9 +++++++++ > > 6 files changed, 55 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 419f9d915c78..8090e3d64f67 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) > > DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); > > return -EINVAL; > > } > > + > > + if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) { > > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n", > > + r->modifier[i], i); > > + return -EINVAL; > > + } > > } > > > > return 0; > > @@ -3337,7 +3343,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, > > struct drm_framebuffer *fb; > > int ret; > > > > - if (r->flags & ~DRM_MODE_FB_INTERLACED) { > > + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { > > DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); > > return ERR_PTR(-EINVAL); > > } > > @@ -3353,6 +3359,12 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, > > return ERR_PTR(-EINVAL); > > } > > > > + if (r->flags & DRM_MODE_FB_MODIFIERS && > > + !dev->mode_config.allow_fb_modifiers) { > > + DRM_DEBUG_KMS("driver does not support fb modifiers\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > ret = framebuffer_check(r); > > if (ret) > > return ERR_PTR(ret); > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > index 3785d66721f2..a6d773a61c2d 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > > else > > req->value = 64; > > break; > > + case DRM_CAP_ADDFB2_MODIFIERS: > > + req->value = dev->mode_config.allow_fb_modifiers; > > + break; > > default: > > return -EINVAL; > > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 019c9b562144..767f7d4cab63 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -1153,6 +1153,9 @@ struct drm_mode_config { > > /* whether async page flip is supported or not */ > > bool async_page_flip; > > > > + /* whether the driver supports fb modifiers */ > > + bool allow_fb_modifiers; > > + > > /* cursor size */ > > uint32_t cursor_width, cursor_height; > > }; > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 01b2d6d0e355..ff6ef62d084b 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -630,6 +630,7 @@ struct drm_gem_open { > > */ > > #define DRM_CAP_CURSOR_WIDTH 0x8 > > #define DRM_CAP_CURSOR_HEIGHT 0x9 > > +#define DRM_CAP_ADDFB2_MODIFIERS 0x10 > > > > /** DRM_IOCTL_GET_CAP ioctl argument type */ > > struct drm_get_cap { > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > > index 646ae5f39f42..bf99557f1624 100644 > > --- a/include/uapi/drm/drm_fourcc.h > > +++ b/include/uapi/drm/drm_fourcc.h > > @@ -132,4 +132,30 @@ > > #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */ > > #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */ > > > > + > > +/* > > + * 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 56 bits are assigned as vendor sees fit. > > + */ > > + > > +/* Vendor Ids: */ > > +#define DRM_FORMAT_MOD_NONE 0 > > +#define DRM_FORMAT_MOD_VENDOR_INTEL 0x01 > > +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02 > > +#define DRM_FORMAT_MOD_VENDOR_NV 0x03 > > +#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04 > > +#define DRM_FORMAT_MOD_VENDOR_QCOM 0x05 > > +/* add more to the end as needed */ > > + > > +#define fourcc_mod_code(vendor, val) \ > > + ((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL) > > + > > +/* Format Modifier tokens: */ > > + > > #endif /* DRM_FOURCC_H */ > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index ca788e01dab2..dbeba949462a 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -336,6 +336,7 @@ struct drm_mode_fb_cmd { > > }; > > > > #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ > > +#define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ > > > > struct drm_mode_fb_cmd2 { > > __u32 fb_id; > > @@ -356,10 +357,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_FORMAT_MOD_xxx. > > */ > > __u32 handles[4]; > > __u32 pitches[4]; /* pitch for each plane */ > > __u32 offsets[4]; /* offset of each plane */ > > + __u64 modifier[4]; /* ie, tiling, compressed (per plane) */ > > }; > > > > #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01 > > -- > > 2.1.4 > >
On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: > > On 01/28/2015 05:37 PM, Daniel Vetter wrote: > >From: Rob Clark <robdclark@gmail.com> > > > >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? > > Maybe: > __u64 modifier[4]; > __u64 vendor_modifier[4]; Seems rendundant since the modifier added in this patch is already vendor specific. Or what exactly are you trying to accomplish here? -Daniel > > ? > > Regards, > > Tvrtko
On 01/29/2015 11:30 AM, Daniel Vetter wrote: > On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: >> >> On 01/28/2015 05:37 PM, Daniel Vetter wrote: >>> From: Rob Clark <robdclark@gmail.com> >>> >>> 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? >> >> Maybe: >> __u64 modifier[4]; >> __u64 vendor_modifier[4]; > > Seems rendundant since the modifier added in this patch is already vendor > specific. Or what exactly are you trying to accomplish here? I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id on the head followed by maybe standardized or maybe vendor specific tag. Feels funny. Would it not be simpler to put a struct in there? But I was not following this from the start so maybe I am missing something.. Regards, Tvrtko
On Thu, Jan 29, 2015 at 11:43:07AM +0000, Tvrtko Ursulin wrote: > > On 01/29/2015 11:30 AM, Daniel Vetter wrote: > >On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: > >> > >>On 01/28/2015 05:37 PM, Daniel Vetter wrote: > >>>From: Rob Clark <robdclark@gmail.com> > >>> > >>>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? > >> > >>Maybe: > >> __u64 modifier[4]; > >> __u64 vendor_modifier[4]; > > > >Seems rendundant since the modifier added in this patch is already vendor > >specific. Or what exactly are you trying to accomplish here? > > I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id > on the head followed by maybe standardized or maybe vendor specific tag. > Feels funny. Would it not be simpler to put a struct in there? The u64 modifier is just an opaque thing, with 8 bit to identifier the vendor (for easier number management really) and the low 56 bits can be whatever we want them. On i915 I think we should just enumerate all the various tiling modes we have. And if the modifiers aren't set we use the tiling mode of the underlying gem bo. We already have code in place to guarantee that the underlying bo's tiling can't change as long as there's a kms fb around, which means all code which checks for tiling can switch over to these flags. struct won't work since by definition this is vendor-specific, and every vendor is slightly insane in a different way. -Daniel
On 01/29/2015 11:57 AM, Daniel Vetter wrote: > On Thu, Jan 29, 2015 at 11:43:07AM +0000, Tvrtko Ursulin wrote: >> >> On 01/29/2015 11:30 AM, Daniel Vetter wrote: >>> On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: >>>> >>>> On 01/28/2015 05:37 PM, Daniel Vetter wrote: >>>>> From: Rob Clark <robdclark@gmail.com> >>>>> >>>>> 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? >>>> >>>> Maybe: >>>> __u64 modifier[4]; >>>> __u64 vendor_modifier[4]; >>> >>> Seems rendundant since the modifier added in this patch is already vendor >>> specific. Or what exactly are you trying to accomplish here? >> >> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id >> on the head followed by maybe standardized or maybe vendor specific tag. >> Feels funny. Would it not be simpler to put a struct in there? > > The u64 modifier is just an opaque thing, with 8 bit to identifier the > vendor (for easier number management really) and the low 56 bits can be > whatever we want them. On i915 I think we should just enumerate all the > various tiling modes we have. And if the modifiers aren't set we use the > tiling mode of the underlying gem bo. We already have code in place to > guarantee that the underlying bo's tiling can't change as long as there's > a kms fb around, which means all code which checks for tiling can switch > over to these flags. > > struct won't work since by definition this is vendor-specific, and every > vendor is slightly insane in a different way. Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits expensive? :) That was first part of my point. Secondly, "vendor who registers first" part of discussion is what came to my attention as well. With this, a hypothetical standard format would be under a vendor id which looks funny to me. While you could split standard formats/modifiers and vendor specific modifiers. I don't care really, it was just an idea, but I don't get this arguments why it is, not only not better, but worse than a bitfield. :) Regards, Tvrtko
On Thu, Jan 29, 2015 at 12:55:48PM +0000, Tvrtko Ursulin wrote: > > On 01/29/2015 11:57 AM, Daniel Vetter wrote: > >On Thu, Jan 29, 2015 at 11:43:07AM +0000, Tvrtko Ursulin wrote: > >> > >>On 01/29/2015 11:30 AM, Daniel Vetter wrote: > >>>On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: > >>>> > >>>>On 01/28/2015 05:37 PM, Daniel Vetter wrote: > >>>>>From: Rob Clark <robdclark@gmail.com> > >>>>> > >>>>>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? > >>>> > >>>>Maybe: > >>>> __u64 modifier[4]; > >>>> __u64 vendor_modifier[4]; > >>> > >>>Seems rendundant since the modifier added in this patch is already vendor > >>>specific. Or what exactly are you trying to accomplish here? > >> > >>I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id > >>on the head followed by maybe standardized or maybe vendor specific tag. > >>Feels funny. Would it not be simpler to put a struct in there? > > > >The u64 modifier is just an opaque thing, with 8 bit to identifier the > >vendor (for easier number management really) and the low 56 bits can be > >whatever we want them. On i915 I think we should just enumerate all the > >various tiling modes we have. And if the modifiers aren't set we use the > >tiling mode of the underlying gem bo. We already have code in place to > >guarantee that the underlying bo's tiling can't change as long as there's > >a kms fb around, which means all code which checks for tiling can switch > >over to these flags. > > > >struct won't work since by definition this is vendor-specific, and every > >vendor is slightly insane in a different way. > > Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits > expensive? :) That was first part of my point. > > Secondly, "vendor who registers first" part of discussion is what came to my > attention as well. > > With this, a hypothetical standard format would be under a vendor id which > looks funny to me. While you could split standard formats/modifiers and > vendor specific modifiers. If some standard body actually manages to pull this off we can always add a new enum value for KHRONOS or MICROSOFT or ISO or whatever it ends up being. The split really is just to make number assignements less conflit-y. > I don't care really, it was just an idea, but I don't get this arguments why > it is, not only not better, but worse than a bitfield. :) Just from your struct without any explanation what your idea was (namely modifiers which are standardized across vendors it seems) it looked like both a plain modifier and a vendor_modifier was possible. Which didn't make a lot of sense to me, so I asked. Going with an opaque u64 has the benefits that it matches kms property values. So if we ever extend this and allow generic properties for framebuffers it'll still fit. A struct is more painful. The idea of fb properties was one of the longer-term ideas tossed around in the v1 thread. -Daniel
On Thu, Jan 29, 2015 at 7:55 AM, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > On 01/29/2015 11:57 AM, Daniel Vetter wrote: >> >> On Thu, Jan 29, 2015 at 11:43:07AM +0000, Tvrtko Ursulin wrote: >>> >>> >>> On 01/29/2015 11:30 AM, Daniel Vetter wrote: >>>> >>>> On Wed, Jan 28, 2015 at 05:57:56PM +0000, Tvrtko Ursulin wrote: >>>>> >>>>> >>>>> On 01/28/2015 05:37 PM, Daniel Vetter wrote: >>>>>> >>>>>> From: Rob Clark <robdclark@gmail.com> >>>>>> >>>>>> 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? >>>>> >>>>> >>>>> Maybe: >>>>> __u64 modifier[4]; >>>>> __u64 vendor_modifier[4]; >>>> >>>> >>>> Seems rendundant since the modifier added in this patch is already >>>> vendor >>>> specific. Or what exactly are you trying to accomplish here? >>> >>> >>> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor >>> id >>> on the head followed by maybe standardized or maybe vendor specific tag. >>> Feels funny. Would it not be simpler to put a struct in there? >> >> >> The u64 modifier is just an opaque thing, with 8 bit to identifier the >> vendor (for easier number management really) and the low 56 bits can be >> whatever we want them. On i915 I think we should just enumerate all the >> various tiling modes we have. And if the modifiers aren't set we use the >> tiling mode of the underlying gem bo. We already have code in place to >> guarantee that the underlying bo's tiling can't change as long as there's >> a kms fb around, which means all code which checks for tiling can switch >> over to these flags. >> >> struct won't work since by definition this is vendor-specific, and every >> vendor is slightly insane in a different way. > > > Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits > expensive? :) That was first part of my point. tbh, we could decide to do something different from 8+56b later if needed.. nothing should really *depend* on the 8+56, since it is intended to be an opaque token. The 8+56 was just intended to make it easier to merge values coming from different driver trees with less conflicts. > Secondly, "vendor who registers first" part of discussion is what came to my > attention as well. > > With this, a hypothetical standard format would be under a vendor id which > looks funny to me. While you could split standard formats/modifiers and > vendor specific modifiers. maybe we should s/vendor/driver/ > I don't care really, it was just an idea, but I don't get this arguments why > it is, not only not better, but worse than a bitfield. :) I guess it gets into bikeshed territory a bit, but I've tried to avoid giving userspace the temptation to assume it is much more than an opaque value. The 8+56 thing was mainly just intended for logistical convenience ;-) BR, -R > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 419f9d915c78..8090e3d64f67 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); return -EINVAL; } + + if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) { + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n", + r->modifier[i], i); + return -EINVAL; + } } return 0; @@ -3337,7 +3343,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, struct drm_framebuffer *fb; int ret; - if (r->flags & ~DRM_MODE_FB_INTERLACED) { + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); return ERR_PTR(-EINVAL); } @@ -3353,6 +3359,12 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev, return ERR_PTR(-EINVAL); } + if (r->flags & DRM_MODE_FB_MODIFIERS && + !dev->mode_config.allow_fb_modifiers) { + DRM_DEBUG_KMS("driver does not support fb modifiers\n"); + return ERR_PTR(-EINVAL); + } + ret = framebuffer_check(r); if (ret) return ERR_PTR(ret); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 3785d66721f2..a6d773a61c2d 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ else req->value = 64; break; + case DRM_CAP_ADDFB2_MODIFIERS: + req->value = dev->mode_config.allow_fb_modifiers; + break; default: return -EINVAL; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 019c9b562144..767f7d4cab63 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1153,6 +1153,9 @@ struct drm_mode_config { /* whether async page flip is supported or not */ bool async_page_flip; + /* whether the driver supports fb modifiers */ + bool allow_fb_modifiers; + /* cursor size */ uint32_t cursor_width, cursor_height; }; diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 01b2d6d0e355..ff6ef62d084b 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -630,6 +630,7 @@ struct drm_gem_open { */ #define DRM_CAP_CURSOR_WIDTH 0x8 #define DRM_CAP_CURSOR_HEIGHT 0x9 +#define DRM_CAP_ADDFB2_MODIFIERS 0x10 /** DRM_IOCTL_GET_CAP ioctl argument type */ struct drm_get_cap { diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 646ae5f39f42..bf99557f1624 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -132,4 +132,30 @@ #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */ #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */ + +/* + * 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 56 bits are assigned as vendor sees fit. + */ + +/* Vendor Ids: */ +#define DRM_FORMAT_MOD_NONE 0 +#define DRM_FORMAT_MOD_VENDOR_INTEL 0x01 +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02 +#define DRM_FORMAT_MOD_VENDOR_NV 0x03 +#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04 +#define DRM_FORMAT_MOD_VENDOR_QCOM 0x05 +/* add more to the end as needed */ + +#define fourcc_mod_code(vendor, val) \ + ((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL) + +/* Format Modifier tokens: */ + #endif /* DRM_FOURCC_H */ diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index ca788e01dab2..dbeba949462a 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -336,6 +336,7 @@ struct drm_mode_fb_cmd { }; #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ +#define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ struct drm_mode_fb_cmd2 { __u32 fb_id; @@ -356,10 +357,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_FORMAT_MOD_xxx. */ __u32 handles[4]; __u32 pitches[4]; /* pitch for each plane */ __u32 offsets[4]; /* offset of each plane */ + __u64 modifier[4]; /* ie, tiling, compressed (per plane) */ }; #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01