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

Commit Message

Daniel Vetter Jan. 29, 2015, 5:01 p.m. UTC

From: Rob Clark <[e-mail]>

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 <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Daniel Stone <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Michel Dänzer <[e-mail]>
Signed-off-by: Rob Clark <[e-mail]> (v1.5)
Reviewed-by: Rob Clark <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 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(-)

Comments

Tvrtko Ursulin Jan. 30, 2015, 10:51 a.m. UTC | #1

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

Rob Clark Jan. 30, 2015, 1:43 p.m. UTC | #2

On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
<[e-mail]> 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

Tvrtko Ursulin Jan. 30, 2015, 2:35 p.m. UTC | #3

On 01/30/2015 01:43 PM, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
> <[e-mail]> 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

Rob Clark Jan. 30, 2015, 2:51 p.m. UTC | #4

On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
<[e-mail]> wrote:
>
> On 01/30/2015 01:43 PM, Rob Clark wrote:
>>
>> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
>> <[e-mail]> 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

Tvrtko Ursulin Jan. 30, 2015, 3:19 p.m. UTC | #5

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

Daniel Vetter Jan. 30, 2015, 3:42 p.m. UTC | #6

On Fri, Jan 30, 2015 at 09:51:48AM -0500, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
> <[e-mail]> wrote:
> >
> > On 01/30/2015 01:43 PM, Rob Clark wrote:
> >>
> >> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
> >> <[e-mail]> 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

Patch

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