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

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

Commit Message

Daniel Vetter Jan. 30, 2015, 4:08 p.m. UTC
From: Rob Clark <robdclark@gmail.com>

In DRM/KMS we are lacking a good way to deal with tiled/compressed
formats.  Especially in the case of dmabuf/prime buffer sharing, where
we cannot always rely on under-the-hood flags passed to driver specific
gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier.  This allows to, if
necessary, use different tiling patters for sub-sampled planes, etc.
The format modifiers are added at the end of the ioctl struct, so for
legacy userspace it will be zero padded.

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

Comments

Thierry Reding Feb. 3, 2015, 3:36 p.m. UTC | #1
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>
Dave Airlie March 11, 2015, 6:40 a.m. UTC | #2
> +
> +#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.

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 419f9d915c78..8090e3d64f67 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3324,6 +3324,12 @@  static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
 			return -EINVAL;
 		}
+
+		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
+			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
+				      r->modifier[i], i);
+			return -EINVAL;
+		}
 	}
 
 	return 0;
@@ -3337,7 +3343,7 @@  static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
 	struct drm_framebuffer *fb;
 	int ret;
 
-	if (r->flags & ~DRM_MODE_FB_INTERLACED) {
+	if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
 		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
 		return ERR_PTR(-EINVAL);
 	}
@@ -3353,6 +3359,12 @@  static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (r->flags & DRM_MODE_FB_MODIFIERS &&
+	    !dev->mode_config.allow_fb_modifiers) {
+		DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	ret = framebuffer_check(r);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_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