[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Rob Clark]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

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.

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.

v3:
- Also add a new ->modifier[] array to struct drm_framebuffer and fill
  it in drm_helper_mode_fill_fb_struct. Requested by Tvrkto Uruslin.
- Remove TODO in comment and add code comment that modifiers should be
  properly documented, requested by Rob.

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)
Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c        | 14 +++++++++++++-
 drivers/gpu/drm/drm_crtc_helper.c |  1 +
 drivers/gpu/drm/drm_ioctl.c       |  3 +++
 include/drm/drm_crtc.h            |  4 ++++
 include/uapi/drm/drm.h            |  1 +
 include/uapi/drm/drm_fourcc.h     | 32 ++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_mode.h       |  9 +++++++++
 7 files changed, 63 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_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index b1979e7bdc88..3053aab968f9 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -837,6 +837,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 	for (i = 0; i < 4; i++) {
 		fb->pitches[i] = mode_cmd->pitches[i];
 		fb->offsets[i] = mode_cmd->offsets[i];
+		fb->modifier[i] = mode_cmd->modifier[i];
 	}
 	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &fb->depth,
 				    &fb->bits_per_pixel);
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..bc5dbe3a4ff6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -202,6 +202,7 @@ struct drm_framebuffer {
 	const struct drm_framebuffer_funcs *funcs;
 	unsigned int pitches[4];
 	unsigned int offsets[4];
+	uint64_t modifier[4];
 	unsigned int width;
 	unsigned int height;
 	/* depth can be 15 or 16 */
@@ -1153,6 +1154,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..05c5328f9889 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -132,4 +132,36 @@
 #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:
+ *
+ * When adding a new token please document the layout with a code comment,
+ * similar to the fourcc codes above. drm_fourcc.h is considered the
+ * authoritative source for all of these.
+ */
+
 #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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/29/2015 05:01 PM, Daniel Vetter wrote:

[snip]

> +
> +/*
> + * 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:
> + *
> + * When adding a new token please document the layout with a code comment,
> + * similar to the fourcc codes above. drm_fourcc.h is considered the
> + * authoritative source for all of these.
> + */

On one side modifiers are supposed to be opaque, but then this suggest 
they are supposed to be added in this file and described. Is that right?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>> +/*
>> + * Format Modifier tokens:
>> + *
>> + * When adding a new token please document the layout with a code
>> comment,
>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
>> + * authoritative source for all of these.
>> + */
>
>
> On one side modifiers are supposed to be opaque, but then this suggest they
> are supposed to be added in this file and described. Is that right?


correct..   opaque as in basically enum values.

We do want a description of the format so when someone comes along and
adds a new value, we have a chance of realizing that it is the same as
an existing value, since there are cases where gpu's from different
vendors can support (for example) the same tiling formats.

BR,
-R
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/30/2015 01:43 PM, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>> +/*
>>> + * Format Modifier tokens:
>>> + *
>>> + * When adding a new token please document the layout with a code
>>> comment,
>>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
>>> + * authoritative source for all of these.
>>> + */
>>
>>
>> On one side modifiers are supposed to be opaque, but then this suggest they
>> are supposed to be added in this file and described. Is that right?
>
>
> correct..   opaque as in basically enum values.
>
> We do want a description of the format so when someone comes along and
> adds a new value, we have a chance of realizing that it is the same as
> an existing value, since there are cases where gpu's from different
> vendors can support (for example) the same tiling formats.

Opaque kind of suggests it is driver private and from that angle 
definitions and descriptions wouldn't belong in the core uapi header. So 
I think that's just confusing and I'd drop opaque from the description 
and just call it a token.

(And another reason why I was suggesting to split the space for 
potential common and vendor/driver specific tokens.)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> On 01/30/2015 01:43 PM, Rob Clark wrote:
>>
>> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>
>>>> +/*
>>>> + * Format Modifier tokens:
>>>> + *
>>>> + * When adding a new token please document the layout with a code
>>>> comment,
>>>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
>>>> + * authoritative source for all of these.
>>>> + */
>>>
>>>
>>>
>>> On one side modifiers are supposed to be opaque, but then this suggest
>>> they
>>> are supposed to be added in this file and described. Is that right?
>>
>>
>>
>> correct..   opaque as in basically enum values.
>>
>> We do want a description of the format so when someone comes along and
>> adds a new value, we have a chance of realizing that it is the same as
>> an existing value, since there are cases where gpu's from different
>> vendors can support (for example) the same tiling formats.
>
>
> Opaque kind of suggests it is driver private and from that angle definitions
> and descriptions wouldn't belong in the core uapi header. So I think that's
> just confusing and I'd drop opaque from the description and just call it a
> token.

hmm, to me, 'opaque' just meant 'do not try to interpret the bits'..
but if that is confusing, I don't mind to just call it a token

> (And another reason why I was suggesting to split the space for potential
> common and vendor/driver specific tokens.)

(which works well until some vendor introduces some format, and later
another vendor adds support for it :-P)

BR,
-R

> Regards,
>
> Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/29/2015 05:01 PM, Daniel Vetter wrote:
> +#define fourcc_mod_code(vendor, val) \
> +	((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL)

Unbalanced parentheses.

Finding them as I go along, sorry! :)

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Fri, Jan 30, 2015 at 09:51:48AM -0500, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> >
> > On 01/30/2015 01:43 PM, Rob Clark wrote:
> >>
> >> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
> >> <tvrtko.ursulin@linux.intel.com> wrote:
> >>>>
> >>>> +/*
> >>>> + * Format Modifier tokens:
> >>>> + *
> >>>> + * When adding a new token please document the layout with a code
> >>>> comment,
> >>>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
> >>>> + * authoritative source for all of these.
> >>>> + */
> >>>
> >>>
> >>>
> >>> On one side modifiers are supposed to be opaque, but then this suggest
> >>> they
> >>> are supposed to be added in this file and described. Is that right?
> >>
> >>
> >>
> >> correct..   opaque as in basically enum values.
> >>
> >> We do want a description of the format so when someone comes along and
> >> adds a new value, we have a chance of realizing that it is the same as
> >> an existing value, since there are cases where gpu's from different
> >> vendors can support (for example) the same tiling formats.
> >
> >
> > Opaque kind of suggests it is driver private and from that angle definitions
> > and descriptions wouldn't belong in the core uapi header. So I think that's
> > just confusing and I'd drop opaque from the description and just call it a
> > token.
> 
> hmm, to me, 'opaque' just meant 'do not try to interpret the bits'..
> but if that is confusing, I don't mind to just call it a token

Yeah could be misunderstood, luckily both the commit message and code
comments already just talk about tokens. So I think we're good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

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.

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.

v3:
- Also add a new ->modifier[] array to struct drm_framebuffer and fill
  it in drm_helper_mode_fill_fb_struct. Requested by Tvrtko Uruslin.
- Remove TODO in comment and add code comment that modifiers should be
  properly documented, requested by Rob.

v4: Balance parens, spotted by Tvrtko.

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)
Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c        | 14 +++++++++++++-
 drivers/gpu/drm/drm_crtc_helper.c |  1 +
 drivers/gpu/drm/drm_ioctl.c       |  3 +++
 include/drm/drm_crtc.h            |  4 ++++
 include/uapi/drm/drm.h            |  1 +
 include/uapi/drm/drm_fourcc.h     | 32 ++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_mode.h       |  9 +++++++++
 7 files changed, 63 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_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index b1979e7bdc88..3053aab968f9 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -837,6 +837,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 	for (i = 0; i < 4; i++) {
 		fb->pitches[i] = mode_cmd->pitches[i];
 		fb->offsets[i] = mode_cmd->offsets[i];
+		fb->modifier[i] = mode_cmd->modifier[i];
 	}
 	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &fb->depth,
 				    &fb->bits_per_pixel);
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..bc5dbe3a4ff6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -202,6 +202,7 @@ struct drm_framebuffer {
 	const struct drm_framebuffer_funcs *funcs;
 	unsigned int pitches[4];
 	unsigned int offsets[4];
+	uint64_t modifier[4];
 	unsigned int width;
 	unsigned int height;
 	/* depth can be 15 or 16 */
@@ -1153,6 +1154,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..622109677747 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -132,4 +132,36 @@
 #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:
+ *
+ * When adding a new token please document the layout with a code comment,
+ * similar to the fourcc codes above. drm_fourcc.h is considered the
+ * authoritative source for all of these.
+ */
+
 #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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5685
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  353/353              353/353
SNB                                  400/422              400/422
IVB              +1-2              485/487              484/487
BYT                                  296/296              296/296
HSW                                  507/508              507/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*IVB  igt_gem_pwrite_pread_snooped-copy-performance      PASS(2, M34)      DMESG_WARN(1, M34)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(5, M34M4)PASS(15, M34M4M21)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(6, M34M4)      DMESG_WARN(1, M34)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5689
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  353/353              353/353
SNB                                  400/422              400/422
IVB              +2-1              485/487              486/487
BYT                                  296/296              296/296
HSW                                  507/508              507/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(6, M34M21)PASS(9, M4M34)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(6, M34M4)PASS(16, M34M4M21)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(7, M34M4)      DMESG_WARN(1, M34)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5678
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -8              353/353              345/353
ILK                                  355/355              355/355
SNB                                  400/422              400/422
IVB              +2                 485/487              487/487
BYT                                  296/296              296/296
HSW              +1                 507/508              508/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_concurrent_blit_gpuX-bcs-gpu-read-after-write-interruptible      PASS(2, M25M7)      FAIL(1, M7)
*PNV  igt_gem_concurrent_blit_prw-rcs-early-read-interruptible      PASS(2, M25M7)      FAIL(1, M7)
*PNV  igt_gem_userptr_blits_coherency-sync      PASS(2, M25M7)      CRASH(1, M7)
*PNV  igt_gem_userptr_blits_coherency-unsync      PASS(2, M25M7)      NRUN(1, M7)
*PNV  igt_gen3_render_linear_blits      PASS(6, M25M23M7)      FAIL(1, M7)
*PNV  igt_gen3_render_mixed_blits      PASS(2, M25M7)      FAIL(1, M7)
*PNV  igt_gen3_render_tiledx_blits      PASS(2, M25M7)      FAIL(1, M7)
*PNV  igt_gen3_render_tiledy_blits      PASS(2, M25M7)      FAIL(1, M7)
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(8, M34M21)PASS(10, M4M34)      PASS(1, M4)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(7, M34M4M21)PASS(18, M34M4M21)      PASS(1, M4)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(2, M40)PASS(22, M40M20)      PASS(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4294 bytes --]

On Fri, Jan 30, 2015 at 05:08:23PM +0100, 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.

"patterns". Alternatively perhaps "modes", which is how I've heard it
referred to most commonly.

> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
> 
> 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.
> 
> v3:
> - Also add a new ->modifier[] array to struct drm_framebuffer and fill
>   it in drm_helper_mode_fill_fb_struct. Requested by Tvrtko Uruslin.
> - Remove TODO in comment and add code comment that modifiers should be
>   properly documented, requested by Rob.
> 
> v4: Balance parens, spotted by Tvrtko.
> 
> 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)
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c        | 14 +++++++++++++-
>  drivers/gpu/drm/drm_crtc_helper.c |  1 +
>  drivers/gpu/drm/drm_ioctl.c       |  3 +++
>  include/drm/drm_crtc.h            |  4 ++++
>  include/uapi/drm/drm.h            |  1 +
>  include/uapi/drm/drm_fourcc.h     | 32 ++++++++++++++++++++++++++++++++
>  include/uapi/drm/drm_mode.h       |  9 +++++++++
>  7 files changed, 63 insertions(+), 1 deletion(-)

Also as discussed on IRC, I think this would be better in a non-DRM
specific header so that we can have a central, cross-subsystem
authority.

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 646ae5f39f42..622109677747 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -132,4 +132,36 @@
>  #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 */
>  
> +

Possibly unintented extra blank line?

> +/*
> + * 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


I think this should be NVIDIA for consistency with other naming in the
kernel, at least on Tegra.

Otherwise:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Dave Airlie]

> +
> +#define fourcc_mod_code(vendor, val) \
> +       ((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL))

eh, yeah no,

/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c: In
function ‘skl_wm_method2’:
/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2631:
warning: integer constant is too large for ‘long’ type
/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2632:
warning: integer constant is too large for ‘long’ type

I think you meant ULL here.

Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel