[PATCH 0/5] i915 fb modifier support, respun

[PATCH 0/5] i915 fb modifier support, respun

[Daniel Vetter]

Hi all,

So this is the very quickly spun together version for my take on fb modifiers.
Aim is to reduce the churn a bit with:
- Only validate fb modifiers and old tiling_mode behaviour in framebuffer_init
  to ensure they are consistent. Don't convert over all the code for old
  platforms.
- Guarantee that X-tiling always implies that the underlying bo is fenced with
  X-tiling mode, too. Otherwise the implications into existing code (e.g.
  adjusting fbc) are a bit too much.
- One draft patch to show how I think we should us fb modifiers: Directly switch
  on them like we do with obj->tiling_mode, no need to have remap/masking
  functions around. At least for now.

This isn't complete since some of the shared code, specifically the fb_align
stuff used by framebuffer_init and the fbdev emulation code still uses
obj->tiling_mode. That needs to be converted into a function which takes u64 fb
modifers (for shared code) and the old code retained in an i9xx_ variant (for
the plane_config readout code for old platforms). Then we can add the skl+ stuff
in another version, together with a if/else.

And then framebuffer_init obvsiouly needs to be extended.

Commments highly welcome.

Cheers, Daniel

Daniel Vetter (3):
  drm/i915: Add fb format modifier support
  drm/i915: Use fb format modifiers in skylake_update_primary_plane
  drm: Also check unused fields for addfb2

Tvrtko Ursulin (2):
  drm/i915: Add tiled framebuffer modifiers
  drm/i915: Announce support for framebuffer modifiers

 drivers/gpu/drm/drm_crtc.c           | 17 +++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++--------
 include/uapi/drm/drm_fourcc.h        | 31 +++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 8 deletions(-)

-- 
2.1.4

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

[PATCH 1/5] drm/i915: Add tiled framebuffer modifiers

[Daniel Vetter]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

To be used from the new addfb2 extension.

v2:
- Drop Intel-specific untiled modfier.
- Move to drm_fourcc.h.
- Document layouts a bit and denote them as platform-specific and not
  useable for cross-driver sharing.
- Add Y-tiling for completeness.
- Drop special docstring markers to avoid confusing kerneldoc.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2)
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 include/uapi/drm/drm_fourcc.h   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 217845951b7f..a027a983c82b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -31,6 +31,7 @@
 #define _I915_DRV_H_
 
 #include <uapi/drm/i915_drm.h>
+#include <uapi/drm/drm_fourcc.h>
 
 #include "i915_reg.h"
 #include "intel_bios.h"
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 622109677747..886814c6f9d2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -164,4 +164,35 @@
  * authoritative source for all of these.
  */
 
+/* Intel framebuffer modifiers */
+
+/*
+ * Intel X-tiling layout
+ *
+ * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
+ * in row-major layout. Within the tile bytes are laid out row-major, with
+ * a platform-dependent stride. On top of that the memory can apply
+ * platform-depending swizzling of some higher address bits into bit6.
+ *
+ * This format is highly platforms specific and not useful for cross-driver
+ * sharing. It exists since on a given platform it does uniquely identify the
+ * layout in a simple way for i915-specific userspace.
+ */
+#define I915_FORMAT_MOD_X_TILED	fourcc_mod_code(INTEL, 1)
+
+/*
+ * Intel Y-tiling layout
+ *
+ * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
+ * in row-major layout. Within the tile bytes are laid out in OWORD (16 bytes)
+ * chunks column-major, with a platform-dependent height. On top of that the
+ * memory can apply platform-depending swizzling of some higher address bits
+ * into bit6.
+ *
+ * This format is highly platforms specific and not useful for cross-driver
+ * sharing. It exists since on a given platform it does uniquely identify the
+ * layout in a simple way for i915-specific userspace.
+ */
+#define I915_FORMAT_MOD_Y_TILED	fourcc_mod_code(INTEL, 1)
+
 #endif /* DRM_FOURCC_H */
-- 
2.1.4

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

Re: [PATCH 1/5] drm/i915: Add tiled framebuffer modifiers

[Tvrtko Ursulin]

On 02/09/2015 06:03 PM, Daniel Vetter wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> To be used from the new addfb2 extension.
>
> v2:
> - Drop Intel-specific untiled modfier.
> - Move to drm_fourcc.h.
> - Document layouts a bit and denote them as platform-specific and not
>    useable for cross-driver sharing.
> - Add Y-tiling for completeness.
> - Drop special docstring markers to avoid confusing kerneldoc.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2)
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>   include/uapi/drm/drm_fourcc.h   | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 217845951b7f..a027a983c82b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -31,6 +31,7 @@
>   #define _I915_DRV_H_
>
>   #include <uapi/drm/i915_drm.h>
> +#include <uapi/drm/drm_fourcc.h>
>
>   #include "i915_reg.h"
>   #include "intel_bios.h"
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 622109677747..886814c6f9d2 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -164,4 +164,35 @@
>    * authoritative source for all of these.
>    */
>
> +/* Intel framebuffer modifiers */
> +
> +/*
> + * Intel X-tiling layout
> + *
> + * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
> + * in row-major layout. Within the tile bytes are laid out row-major, with
> + * a platform-dependent stride. On top of that the memory can apply
> + * platform-depending swizzling of some higher address bits into bit6.
> + *
> + * This format is highly platforms specific and not useful for cross-driver
> + * sharing. It exists since on a given platform it does uniquely identify the
> + * layout in a simple way for i915-specific userspace.
> + */
> +#define I915_FORMAT_MOD_X_TILED	fourcc_mod_code(INTEL, 1)
> +
> +/*
> + * Intel Y-tiling layout
> + *
> + * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
> + * in row-major layout. Within the tile bytes are laid out in OWORD (16 bytes)
> + * chunks column-major, with a platform-dependent height. On top of that the
> + * memory can apply platform-depending swizzling of some higher address bits
> + * into bit6.
> + *
> + * This format is highly platforms specific and not useful for cross-driver
> + * sharing. It exists since on a given platform it does uniquely identify the
> + * layout in a simple way for i915-specific userspace.
> + */
> +#define I915_FORMAT_MOD_Y_TILED	fourcc_mod_code(INTEL, 1)

X was one, so this could be two.

Regards,

Tvrtko


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

[PATCH] drm/i915: Add tiled framebuffer modifiers

[Daniel Vetter]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

To be used from the new addfb2 extension.

v2:
- Drop Intel-specific untiled modfier.
- Move to drm_fourcc.h.
- Document layouts a bit and denote them as platform-specific and not
  useable for cross-driver sharing.
- Add Y-tiling for completeness.
- Drop special docstring markers to avoid confusing kerneldoc.

v3: Give Y-tiling a unique idea, noticed by Tvrtko.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> (v1)
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 include/uapi/drm/drm_fourcc.h   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3da3dc527315..99b25928df2f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -31,6 +31,7 @@
 #define _I915_DRV_H_
 
 #include <uapi/drm/i915_drm.h>
+#include <uapi/drm/drm_fourcc.h>
 
 #include "i915_reg.h"
 #include "intel_bios.h"
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 622109677747..4837c3d2319a 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -164,4 +164,35 @@
  * authoritative source for all of these.
  */
 
+/* Intel framebuffer modifiers */
+
+/*
+ * Intel X-tiling layout
+ *
+ * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
+ * in row-major layout. Within the tile bytes are laid out row-major, with
+ * a platform-dependent stride. On top of that the memory can apply
+ * platform-depending swizzling of some higher address bits into bit6.
+ *
+ * This format is highly platforms specific and not useful for cross-driver
+ * sharing. It exists since on a given platform it does uniquely identify the
+ * layout in a simple way for i915-specific userspace.
+ */
+#define I915_FORMAT_MOD_X_TILED	fourcc_mod_code(INTEL, 1)
+
+/*
+ * Intel Y-tiling layout
+ *
+ * This is a tiled layout using 4Kb tiles (except on gen2 where the tiles 2Kb)
+ * in row-major layout. Within the tile bytes are laid out in OWORD (16 bytes)
+ * chunks column-major, with a platform-dependent height. On top of that the
+ * memory can apply platform-depending swizzling of some higher address bits
+ * into bit6.
+ *
+ * This format is highly platforms specific and not useful for cross-driver
+ * sharing. It exists since on a given platform it does uniquely identify the
+ * layout in a simple way for i915-specific userspace.
+ */
+#define I915_FORMAT_MOD_Y_TILED	fourcc_mod_code(INTEL, 2)
+
 #endif /* DRM_FOURCC_H */
-- 
2.1.4

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

[PATCH] drm/i915: Add fb format modifier support

[Daniel Vetter]

Currently we don't support anything but X tiled. And for an easier
transition it makes a lot of sense to just keep requiring that X tiled
is properly fenced.

Which means we need to do absolutely nothing in old code to support fb
modifiers, yay!

v2: Fix the Y tiling check, noticed by Tvrtko.

v3: Catch Y-tiled fb for legacy addfb again (Tvrtko) and explain why
we want X tiling to match in the comment.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2655b63d65e9..da827568671e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12706,7 +12706,24 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->tiling_mode == I915_TILING_Y) {
+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+		/* Enforce that fb modifier and tiling mode match, but only for
+		 * X-tiled. This is needed for FBC. */
+		if (!!(obj->tiling_mode == I915_TILING_X) !=
+		    !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
+			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
+			return -EINVAL;
+		}
+	} else {
+		if (obj->tiling_mode == I915_TILING_X)
+			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
+		else if (obj->tiling_mode == I915_TILING_Y) {
+			DRM_DEBUG("No Y tiling for legacy addfb\n");
+			return -EINVAL;
+		}
+	}
+
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
 		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
 	}
@@ -12720,12 +12737,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
 		pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 4) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 16*1024;
 		else
 			pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 3) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 8*1024;
 		else
 			pitch_limit = 16*1024;
@@ -12735,12 +12752,13 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	if (mode_cmd->pitches[0] > pitch_limit) {
 		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
-			  obj->tiling_mode ? "tiled" : "linear",
+			  mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED ?
+			  "tiled" : "linear",
 			  mode_cmd->pitches[0], pitch_limit);
 		return -EINVAL;
 	}
 
-	if (obj->tiling_mode != I915_TILING_NONE &&
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED &&
 	    mode_cmd->pitches[0] != obj->stride) {
 		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
 			  mode_cmd->pitches[0], obj->stride);
-- 
2.1.4

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

[PATCH 2/5] drm/i915: Add fb format modifier support

[Daniel Vetter]

Currently we don't support anything but X tiled. And for an easier
transition it makes a lot of sense to just keep requiring that X tiled
is properly fenced.

Which means we need to do absolutely nothing in old code to support fb
modifiers, yay!

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3fe95982be93..2d69cce03ab5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12707,7 +12707,20 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->tiling_mode == I915_TILING_Y) {
+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+		/* Enforce that fb modifier and tiling mode match, but only for
+		 * X-tiled. */
+		if (!!(obj->tiling_mode == I915_TILING_X) !=
+		    !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
+			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
+			return -EINVAL;
+		}
+	} else {
+		if (obj->tiling_mode == I915_TILING_X)
+			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
+	}
+
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED) {
 		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
 	}
@@ -12721,12 +12734,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
 		pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 4) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 16*1024;
 		else
 			pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 3) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 8*1024;
 		else
 			pitch_limit = 16*1024;
@@ -12736,12 +12749,13 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	if (mode_cmd->pitches[0] > pitch_limit) {
 		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
-			  obj->tiling_mode ? "tiled" : "linear",
+			  mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED ?
+			  "tiled" : "linear",
 			  mode_cmd->pitches[0], pitch_limit);
 		return -EINVAL;
 	}
 
-	if (obj->tiling_mode != I915_TILING_NONE &&
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED &&
 	    mode_cmd->pitches[0] != obj->stride) {
 		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
 			  mode_cmd->pitches[0], obj->stride);
-- 
2.1.4

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

Re: [PATCH 2/5] drm/i915: Add fb format modifier support

[Tvrtko Ursulin]

On 02/09/2015 06:03 PM, Daniel Vetter wrote:
> Currently we don't support anything but X tiled. And for an easier
> transition it makes a lot of sense to just keep requiring that X tiled
> is properly fenced.
>
> Which means we need to do absolutely nothing in old code to support fb
> modifiers, yay!
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3fe95982be93..2d69cce03ab5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12707,7 +12707,20 @@ static int intel_framebuffer_init(struct drm_device *dev,
>
>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> -	if (obj->tiling_mode == I915_TILING_Y) {
> +	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> +		/* Enforce that fb modifier and tiling mode match, but only for
> +		 * X-tiled. */
> +		if (!!(obj->tiling_mode == I915_TILING_X) !=
> +		    !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
> +			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (obj->tiling_mode == I915_TILING_X)
> +			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> +	}
> +
> +	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED) {
>   		DRM_DEBUG("hardware does not support tiling Y\n");
>   		return -EINVAL;

== I915_FORMAT_MOD_Y_TILED, although it can't really happen with these 
changes. But don't we still need the check against obj->tiling_mode for Y?

Regards,

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

[PATCH] drm/i915: Add fb format modifier support

[Daniel Vetter]

Currently we don't support anything but X tiled. And for an easier
transition it makes a lot of sense to just keep requiring that X tiled
is properly fenced.

Which means we need to do absolutely nothing in old code to support fb
modifiers, yay!

v2: Fix the Y tiling check, noticed by Tvrtko.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2655b63d65e9..26fe302b27a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12706,7 +12706,20 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->tiling_mode == I915_TILING_Y) {
+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+		/* Enforce that fb modifier and tiling mode match, but only for
+		 * X-tiled. */
+		if (!!(obj->tiling_mode == I915_TILING_X) !=
+		    !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
+			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
+			return -EINVAL;
+		}
+	} else {
+		if (obj->tiling_mode == I915_TILING_X)
+			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
+	}
+
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
 		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
 	}
@@ -12720,12 +12733,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
 		pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 4) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 16*1024;
 		else
 			pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 3) {
-		if (obj->tiling_mode)
+		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
 			pitch_limit = 8*1024;
 		else
 			pitch_limit = 16*1024;
@@ -12735,12 +12748,13 @@ static int intel_framebuffer_init(struct drm_device *dev,
 
 	if (mode_cmd->pitches[0] > pitch_limit) {
 		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
-			  obj->tiling_mode ? "tiled" : "linear",
+			  mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED ?
+			  "tiled" : "linear",
 			  mode_cmd->pitches[0], pitch_limit);
 		return -EINVAL;
 	}
 
-	if (obj->tiling_mode != I915_TILING_NONE &&
+	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED &&
 	    mode_cmd->pitches[0] != obj->stride) {
 		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
 			  mode_cmd->pitches[0], obj->stride);
-- 
2.1.4

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

[PATCH 3/5] drm/i915: Use fb format modifiers in skylake_update_primary_plane

[Daniel Vetter]

Just a little demo really. We probably need to introduce skl specific
functions for a lot of the format validation stuff, or at least
helpers. Specifically I think intel_framebuffer_init and
intel_fb_align_height must be adjusted to have an i915_ and a skl_
variant. And only shared code should be converted to fb modifiers,
platform code (like the plane config readout can keep on using old
tiling_mode defines to avoid some churn).

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2d69cce03ab5..41b3ddc4068d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2773,11 +2773,11 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	 * The stride is either expressed as a multiple of 64 bytes chunks for
 	 * linear buffers or in number of tiles for tiled buffers.
 	 */
-	switch (obj->tiling_mode) {
-	case I915_TILING_NONE:
+	switch (fb->modifier[0]) {
+	case DRM_FORMAT_MOD_NONE:
 		stride = fb->pitches[0] >> 6;
 		break;
-	case I915_TILING_X:
+	case I915_FORMAT_MOD_X_TILED:
 		plane_ctl |= PLANE_CTL_TILED_X;
 		stride = fb->pitches[0] >> 9;
 		break;
-- 
2.1.4

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

Re: [PATCH 3/5] drm/i915: Use fb format modifiers in skylake_update_primary_plane

[Damien Lespiau]

On Mon, Feb 09, 2015 at 07:03:26PM +0100, Daniel Vetter wrote:
> Just a little demo really. We probably need to introduce skl specific
> functions for a lot of the format validation stuff, or at least
> helpers. Specifically I think intel_framebuffer_init and
> intel_fb_align_height must be adjusted to have an i915_ and a skl_
> variant. And only shared code should be converted to fb modifiers,
> platform code (like the plane config readout can keep on using old
> tiling_mode defines to avoid some churn).
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2d69cce03ab5..41b3ddc4068d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2773,11 +2773,11 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	 * The stride is either expressed as a multiple of 64 bytes chunks for
>  	 * linear buffers or in number of tiles for tiled buffers.
>  	 */
> -	switch (obj->tiling_mode) {
> -	case I915_TILING_NONE:
> +	switch (fb->modifier[0]) {
> +	case DRM_FORMAT_MOD_NONE:
>  		stride = fb->pitches[0] >> 6;
>  		break;
> -	case I915_TILING_X:
> +	case I915_FORMAT_MOD_X_TILED:
>  		plane_ctl |= PLANE_CTL_TILED_X;
>  		stride = fb->pitches[0] >> 9;
>  		break;

Just a remark across the board is that this code won't work as we add
new types of modifiers besides tiling. Might as wels reserve tiling bits
today and select on them.

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

[PATCH 4/5] drm/i915: Announce support for framebuffer modifiers

[Daniel Vetter]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Let the DRM core know we can handle it.

v2: Change to boolean true. (Daniel Vetter)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41b3ddc4068d..e96d0c75f89c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13214,6 +13214,8 @@ void intel_modeset_init(struct drm_device *dev)
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.prefer_shadow = 1;
 
+	dev->mode_config.allow_fb_modifiers = true;
+
 	dev->mode_config.funcs = &intel_mode_funcs;
 
 	intel_init_quirks(dev);
-- 
2.1.4

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

[PATCH 5/5] drm: Also check unused fields for addfb2

[Daniel Vetter]

Just the usual paranoia ...

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b15d720eda4c..a12d7e8a0ca0 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 		}
 	}
 
+	for (; i < 4; i++) {
+		if (r->handles[i]) {
+			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
+			return -EINVAL;
+		}
+
+		if (r->pitches[i] || r->offsets[i]) {
+			DRM_DEBUG_KMS("buffer pitch/offset for unused plane", i);
+			return -EINVAL;
+		}
+
+		if (r->modifier[i]) {
+			DRM_DEBUG_KMS("fb modifer for unused plane", i);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.1.4

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

[PATCH 1/2] drm/i915: Set up fb format modifier for initial plane config

[Daniel Vetter]

No functional changes yet since intel_framebuffer_init would have
fixed this up for us. But this is prep work to be able to handle new
tiling layouts in the initial plane config code.

Follow-up patches will start to make use of this and switch over to fb
modifiers where needed.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e96d0c75f89c..e2c70f24c841 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2390,6 +2390,8 @@ intel_alloc_plane_obj(struct intel_crtc *crtc,
 	mode_cmd.width = fb->width;
 	mode_cmd.height = fb->height;
 	mode_cmd.pitches[0] = fb->pitches[0];
+	mode_cmd.modifier[0] = fb->modifier[0];
+	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -6625,9 +6627,12 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 
 	fb = &intel_fb->base;
 
-	if (INTEL_INFO(dev)->gen >= 4)
-		if (val & DISPPLANE_TILED)
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (val & DISPPLANE_TILED) {
 			plane_config->tiling = I915_TILING_X;
+			fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
+		}
+	}
 
 	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
 	fourcc = i9xx_format_to_fourcc(pixel_format);
@@ -7659,8 +7664,10 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	if (!(val & PLANE_CTL_ENABLE))
 		goto error;
 
-	if (val & PLANE_CTL_TILED_MASK)
+	if (val & PLANE_CTL_TILED_MASK) {
 		plane_config->tiling = I915_TILING_X;
+		fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
+	}
 
 	pixel_format = val & PLANE_CTL_FORMAT_MASK;
 	fourcc = skl_format_to_fourcc(pixel_format,
@@ -7758,9 +7765,12 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 
 	fb = &intel_fb->base;
 
-	if (INTEL_INFO(dev)->gen >= 4)
-		if (val & DISPPLANE_TILED)
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (val & DISPPLANE_TILED) {
 			plane_config->tiling = I915_TILING_X;
+			fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
+		}
+	}
 
 	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
 	fourcc = i9xx_format_to_fourcc(pixel_format);
-- 
2.1.4

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

[PATCH 2/2] drm/i915: Switch +intel_fb_align_height to fb format modifiers

[Daniel Vetter]

With this we can treat the fb format modifier completely independently
from the fencing mode in obj->tiling_mode in the initial plane code.
Which means new tiling modes without any gtt fence are now fully
support in the core i915 driver code.

v2: Also add pixel_format while at it, we need this to compute the
height for the new tiling formats.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_fbdev.c   |  3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2c70f24c841..56e4a66e0683 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2190,11 +2190,15 @@ static bool need_vtd_wa(struct drm_device *dev)
 }
 
 int
-intel_fb_align_height(struct drm_device *dev, int height, unsigned int tiling)
+intel_fb_align_height(struct drm_device *dev, int height,
+		      uint32_t pixel_format,
+		      uint64_t fb_format_modifier)
 {
 	int tile_height;
 
-	tile_height = tiling ? (IS_GEN2(dev) ? 16 : 8) : 1;
+	tile_height = fb_format_modifier == I915_FORMAT_MOD_X_TILED ?
+		(IS_GEN2(dev) ? 16 : 8) : 1;
+
 	return ALIGN(height, tile_height);
 }
 
@@ -6658,7 +6662,8 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 	fb->pitches[0] = val & 0xffffffc0;
 
 	aligned_height = intel_fb_align_height(dev, fb->height,
-					       plane_config->tiling);
+					       fb->pixel_format,
+					       fb->modifier[0]);
 
 	plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
 
@@ -7700,7 +7705,8 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	fb->pitches[0] = (val & 0x3ff) * stride_mult;
 
 	aligned_height = intel_fb_align_height(dev, fb->height,
-					       plane_config->tiling);
+					       fb->pixel_format,
+					       fb->modifier[0]);
 
 	plane_config->size = ALIGN(fb->pitches[0] * aligned_height, PAGE_SIZE);
 
@@ -7796,7 +7802,8 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 	fb->pitches[0] = val & 0xffffffc0;
 
 	aligned_height = intel_fb_align_height(dev, fb->height,
-					       plane_config->tiling);
+					       fb->pixel_format,
+					       fb->modifier[0]);
 
 	plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
 
@@ -12820,7 +12827,8 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		return -EINVAL;
 
 	aligned_height = intel_fb_align_height(dev, mode_cmd->height,
-					       obj->tiling_mode);
+					       mode_cmd->pixel_format,
+					       mode_cmd->modifier[0]);
 	/* FIXME drm helper for size checks (especially planar formats)? */
 	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76b3c2043954..b9598ba6901c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -879,7 +879,8 @@ void intel_frontbuffer_flip(struct drm_device *dev,
 }
 
 int intel_fb_align_height(struct drm_device *dev, int height,
-			  unsigned int tiling);
+			  uint32_t pixel_format,
+			  uint64_t fb_format_modifier);
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 3001a8674611..234a699b8219 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -594,7 +594,8 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 
 		cur_size = intel_crtc->config->base.adjusted_mode.crtc_vdisplay;
 		cur_size = intel_fb_align_height(dev, cur_size,
-						 plane_config->tiling);
+						 fb->base.pixel_format,
+						 fb->base.modifier[0]);
 		cur_size *= fb->base.pitches[0];
 		DRM_DEBUG_KMS("pipe %c area: %dx%d, bpp: %d, size: %d\n",
 			      pipe_name(intel_crtc->pipe),
-- 
2.1.4

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

Re: [PATCH 5/5] drm: Also check unused fields for addfb2

[Chris Wilson]

On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote:
> Just the usual paranoia ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b15d720eda4c..a12d7e8a0ca0 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  		}
>  	}
>  
> +	for (; i < 4; i++) {
> +		if (r->handles[i]) {
> +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);

Printing the invalid value is also useful. We tended to put user
debugging messages as DRM_DEBUG(); would probably be useful to add
DRM_DEBUG_USER() and clean up all the EINVAL reporting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm: Also check unused fields for addfb2

[Daniel Vetter]

On Tue, Feb 10, 2015 at 11:01:56AM +0000, Chris Wilson wrote:
> On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote:
> > Just the usual paranoia ...
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b15d720eda4c..a12d7e8a0ca0 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >  		}
> >  	}
> >  
> > +	for (; i < 4; i++) {
> > +		if (r->handles[i]) {
> > +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
> 
> Printing the invalid value is also useful. We tended to put user
> debugging messages as DRM_DEBUG(); would probably be useful to add
> DRM_DEBUG_USER() and clean up all the EINVAL reporting.

Generally I agree, but here I couldn't come up with a case where it would
be useful:
- Missing memset is just that.
- Memset is there, but userspace filled in too many buffers - the i it
  prints should be enough.

-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 5/5] drm: Also check unused fields for addfb2

[Chris Wilson]

On Tue, Feb 10, 2015 at 12:36:51PM +0100, Daniel Vetter wrote:
> On Tue, Feb 10, 2015 at 11:01:56AM +0000, Chris Wilson wrote:
> > On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote:
> > > Just the usual paranoia ...
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index b15d720eda4c..a12d7e8a0ca0 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > >  		}
> > >  	}
> > >  
> > > +	for (; i < 4; i++) {
> > > +		if (r->handles[i]) {
> > > +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
> > 
> > Printing the invalid value is also useful. We tended to put user
> > debugging messages as DRM_DEBUG(); would probably be useful to add
> > DRM_DEBUG_USER() and clean up all the EINVAL reporting.
> 
> Generally I agree, but here I couldn't come up with a case where it would
> be useful:
> - Missing memset is just that.
> - Memset is there, but userspace filled in too many buffers - the i it
>   prints should be enough.

The comment was more towards the future, when I expect the validity
checking to be more stringent. For consistency we should use the same
debug level as for other EINVAL logging, and when we come to cut and
paste the error message, having the value in there should remind us to
be verbose.

Even here I expect the value to be useful diagnostic, e.g. to help
narrow down the circumstances under which the invalid ioctl was called.
Maybe it contains poison, maybe it is valid but stale etc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx