RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)
diff mbox

Message ID 1422466672-15833-1-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter Jan. 28, 2015, 5:37 p.m. UTC
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?

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>

--

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(-)

Comments

Tvrtko Ursulin Jan. 28, 2015, 5:57 p.m. UTC | #1
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
Rob Clark Jan. 28, 2015, 6:46 p.m. UTC | #2
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
>
Daniel Vetter Jan. 29, 2015, 11:29 a.m. UTC | #3
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
> >
Daniel Vetter Jan. 29, 2015, 11:30 a.m. UTC | #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
Tvrtko Ursulin Jan. 29, 2015, 11:43 a.m. UTC | #5
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
Daniel Vetter Jan. 29, 2015, 11:57 a.m. UTC | #6
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
Tvrtko Ursulin Jan. 29, 2015, 12:55 p.m. UTC | #7
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
Daniel Vetter Jan. 29, 2015, 1:27 p.m. UTC | #8
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
Rob Clark Jan. 29, 2015, 3:09 p.m. UTC | #9
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

Patch
diff mbox

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