[PATCH v4 0/8] Skylake Y tiled scanout

[PATCH v4 0/8] Skylake Y tiled scanout

[Tvrtko Ursulin]

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

Starting with Skylake the display engine can scan out Y tiled objects. (Both
legacy Y tiled, and the new Yf format.)

This series takes the original work by Damien Lespiau and converts it to use the
new frame buffer modifiers instead of object set/get tiling. Some patches needed
to be dropped, some added and some refactored.

Lightly tested with "testdisplay -m -y" and "testdisplay -m --yf".

v2: Rebased on v2 of "i915 fb modifier support, respun".

v3:
   * Part which allows Y tiled fb creation extracted out and moved to the end
     of series.
   * Dropped redundant "drm/i915/skl: Allow Y tiling for sprites".
   * Also see individual change logs.

v4:
   * New to the series - watermark programming updates per BSpec.
   * Addressed review comments in individual patches.

Damien Lespiau (4):
  drm/i915/skl: Allow scanning out Y and Yf fbs
  drm/i915/skl: Adjust intel_fb_align_height() for Yb/Yf tiling
  drm/i915/skl: Teach pin_and_fence_fb_obj() about Y tiling constraints
  drm/i915/skl: Adjust get_plane_config() to support Yb/Yf tiling

Tvrtko Ursulin (4):
  drm/i915/skl: Add new displayable tiling formats
  drm/i915/skl: Updated watermark programming
  drm/i915/skl: Update watermarks for Y tiling
  drm/i915/skl: Allow Y (and Yf) frame buffer creation

 drivers/gpu/drm/i915/intel_display.c | 240 ++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_pm.c      |  96 ++++++++++----
 drivers/gpu/drm/i915/intel_sprite.c  |  24 +++-
 include/uapi/drm/drm_fourcc.h        |  15 +++
 5 files changed, 289 insertions(+), 89 deletions(-)

-- 
2.3.0

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

[PATCH 1/8] drm/i915/skl: Add new displayable tiling formats

[Tvrtko Ursulin]

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

Starting with SKL display engine can scan out Y, and newly introduced Yf
tiling formats so add the latter to the frame buffer modifier space.

v2: Definitions moved to drm_fourcc.h.
v3: Try to document the format better.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/uapi/drm/drm_fourcc.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 1a5a357..e6efac2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -192,4 +192,19 @@
  */
 #define I915_FORMAT_MOD_Y_TILED	fourcc_mod_code(INTEL, 2)
 
+/*
+ * Intel Yf-tiling layout
+ *
+ * This is a tiled layout using 4Kb tiles in row-major layout.
+ * Within the tile pixels are laid out in 16 256 byte units / sub-tiles which
+ * are arranged in four groups (two wide, two high) with column-major layout.
+ * Each group therefore consits out of four 256 byte units, which are also laid
+ * out as 2x2 column-major.
+ * 256 byte units are made out of four 64 byte blocks of pixels, producing
+ * either a square block or a 2:1 unit.
+ * 64 byte blocks of pixels contain four pixel rows of 16 bytes, where the width
+ * in pixel depends on the pixel depth.
+ */
+#define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
+
 #endif /* DRM_FOURCC_H */
-- 
2.3.0

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

[PATCH 2/8] drm/i915/skl: Allow scanning out Y and Yf fbs

[Tvrtko Ursulin]

From: Damien Lespiau <damien.lespiau@intel.com>

Skylake is able to scannout those tiling formats. We need to allow them
in the ADDFB ioctl and tell the harware about it.

v2: Rebased for addfb2 interface. (Tvrtko Ursulin)
v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
v4: Don't allow Y tiled fbs just yet. (Tvrtko Ursulin)
v5: Check for stride alignment and max pitch. (Tvrtko Ursulin)
v6: Simplify maximum pitch check. (Ville Syrjälä)
v7: Drop the gen9 check since requirements are no different. (Ville Syrjälä)
v8: Gen2 has different X tiling stride. (Ville Syrjälä)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v7)
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 121 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
 3 files changed, 101 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e70748..ae0467a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2728,6 +2728,40 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 	POSTING_READ(reg);
 }
 
+u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
+			      uint32_t pixel_format)
+{
+	u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
+
+	/*
+	 * The stride is either expressed as a multiple of 64 bytes
+	 * chunks for linear buffers or in number of tiles for tiled
+	 * buffers.
+	 */
+	switch (fb_modifier) {
+	case DRM_FORMAT_MOD_NONE:
+		return 64;
+	case I915_FORMAT_MOD_X_TILED:
+		if (INTEL_INFO(dev)->gen == 2)
+			return 128;
+		return 512;
+	case I915_FORMAT_MOD_Y_TILED:
+		/* No need to check for old gens and Y tiling since this is
+		 * about the display engine and those will be blocked before
+		 * we get here.
+		 */
+		return 128;
+	case I915_FORMAT_MOD_Yf_TILED:
+		if (bits_per_pixel == 8)
+			return 64;
+		else
+			return 128;
+	default:
+		MISSING_CASE(fb_modifier);
+		return 64;
+	}
+}
+
 static void skylake_update_primary_plane(struct drm_crtc *crtc,
 					 struct drm_framebuffer *fb,
 					 int x, int y)
@@ -2735,10 +2769,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb;
 	struct drm_i915_gem_object *obj;
 	int pipe = intel_crtc->pipe;
-	u32 plane_ctl, stride;
+	u32 plane_ctl, stride_div;
 
 	if (!intel_crtc->primary_enabled) {
 		I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -2782,29 +2815,30 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		BUG();
 	}
 
-	intel_fb = to_intel_framebuffer(fb);
-	obj = intel_fb->obj;
-
-	/*
-	 * The stride is either expressed as a multiple of 64 bytes chunks for
-	 * linear buffers or in number of tiles for tiled buffers.
-	 */
 	switch (fb->modifier[0]) {
 	case DRM_FORMAT_MOD_NONE:
-		stride = fb->pitches[0] >> 6;
 		break;
 	case I915_FORMAT_MOD_X_TILED:
 		plane_ctl |= PLANE_CTL_TILED_X;
-		stride = fb->pitches[0] >> 9;
+		break;
+	case I915_FORMAT_MOD_Y_TILED:
+		plane_ctl |= PLANE_CTL_TILED_Y;
+		break;
+	case I915_FORMAT_MOD_Yf_TILED:
+		plane_ctl |= PLANE_CTL_TILED_YF;
 		break;
 	default:
-		BUG();
+		MISSING_CASE(fb->modifier[0]);
 	}
 
 	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
 	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
 		plane_ctl |= PLANE_CTL_ROTATE_180;
 
+	obj = intel_fb_obj(fb);
+	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
+					       fb->pixel_format);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 
 	DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
@@ -2817,7 +2851,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	I915_WRITE(PLANE_SIZE(pipe, 0),
 		   (intel_crtc->config->pipe_src_h - 1) << 16 |
 		   (intel_crtc->config->pipe_src_w - 1));
-	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
+	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
 	I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
 
 	POSTING_READ(PLANE_SURF(pipe, 0));
@@ -12657,14 +12691,43 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
 	.create_handle = intel_user_framebuffer_create_handle,
 };
 
+static
+u32 intel_fb_pitch_limit(struct drm_device *dev, uint64_t fb_modifier,
+			 uint32_t pixel_format)
+{
+	u32 gen = INTEL_INFO(dev)->gen;
+
+	if (gen >= 9) {
+		/* "The stride in bytes must not exceed the of the size of 8K
+		 *  pixels and 32K bytes."
+		 */
+		 return min(8192*drm_format_plane_cpp(pixel_format, 0), 32768);
+	} else if (gen >= 5 && !IS_VALLEYVIEW(dev)) {
+		return 32*1024;
+	} else if (gen >= 4) {
+		if (fb_modifier == I915_FORMAT_MOD_X_TILED)
+			return 16*1024;
+		else
+			return 32*1024;
+	} else if (gen >= 3) {
+		if (fb_modifier == I915_FORMAT_MOD_X_TILED)
+			return 8*1024;
+		else
+			return 16*1024;
+	} else {
+		/* XXX DSPC is limited to 4k tiled */
+		return 8*1024;
+	}
+}
+
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *intel_fb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
 				  struct drm_i915_gem_object *obj)
 {
 	int aligned_height;
-	int pitch_limit;
 	int ret;
+	u32 pitch_limit, stride_alignment;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
@@ -12690,31 +12753,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	if (mode_cmd->pitches[0] & 63) {
-		DRM_DEBUG("pitch (%d) must be at least 64 byte aligned\n",
-			  mode_cmd->pitches[0]);
+	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
+						     mode_cmd->pixel_format);
+	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
+		DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
+			  mode_cmd->pitches[0], stride_alignment);
 		return -EINVAL;
 	}
 
-	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
-		pitch_limit = 32*1024;
-	} else if (INTEL_INFO(dev)->gen >= 4) {
-		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 (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)
-			pitch_limit = 8*1024;
-		else
-			pitch_limit = 16*1024;
-	} else
-		/* XXX DSPC is limited to 4k tiled */
-		pitch_limit = 8*1024;
-
+	pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0],
+					   mode_cmd->pixel_format);
 	if (mode_cmd->pitches[0] > pitch_limit) {
-		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
-			  mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED ?
+		DRM_DEBUG("%s pitch (%u) must be at less than %d\n",
+			  mode_cmd->modifier[0] != DRM_FORMAT_MOD_NONE ?
 			  "tiled" : "linear",
 			  mode_cmd->pitches[0], pitch_limit);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1de8e20..399d2b2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -883,6 +883,8 @@ int intel_fb_align_height(struct drm_device *dev, int height,
 			  uint64_t fb_format_modifier);
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 
+u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
+			      uint32_t pixel_format);
 
 /* intel_audio.c */
 void intel_init_audio(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 5ae56ec..b7103bd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -189,7 +189,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
-	u32 plane_ctl, stride;
+	u32 plane_ctl, stride_div;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 
 	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
@@ -247,15 +247,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	switch (fb->modifier[0]) {
 	case DRM_FORMAT_MOD_NONE:
-		stride = fb->pitches[0] >> 6;
 		break;
 	case I915_FORMAT_MOD_X_TILED:
 		plane_ctl |= PLANE_CTL_TILED_X;
-		stride = fb->pitches[0] >> 9;
+		break;
+	case I915_FORMAT_MOD_Y_TILED:
+		plane_ctl |= PLANE_CTL_TILED_Y;
+		break;
+	case I915_FORMAT_MOD_Yf_TILED:
+		plane_ctl |= PLANE_CTL_TILED_YF;
 		break;
 	default:
-		BUG();
+		MISSING_CASE(fb->modifier[0]);
 	}
+
 	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
 		plane_ctl |= PLANE_CTL_ROTATE_180;
 
@@ -266,6 +271,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 				       pixel_size, true,
 				       src_w != crtc_w || src_h != crtc_h);
 
+	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
+					       fb->pixel_format);
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -273,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	crtc_h--;
 
 	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
-	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
+	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
 	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
 	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
 	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
-- 
2.3.0

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

[PATCH 3/8] drm/i915/skl: Adjust intel_fb_align_height() for Yb/Yf tiling

[Tvrtko Ursulin]

From: Damien Lespiau <damien.lespiau@intel.com>

We now need the bpp of the fb as Yf tiling has different tile widths
depending on it.

v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
v4: Added missing case and 128-bit pixel warning. (Damien Lespiau)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v3)
---
 drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ae0467a..004e980 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2195,9 +2195,44 @@ intel_fb_align_height(struct drm_device *dev, int height,
 		      uint64_t fb_format_modifier)
 {
 	int tile_height;
+	uint32_t bits_per_pixel;
 
-	tile_height = fb_format_modifier == I915_FORMAT_MOD_X_TILED ?
-		(IS_GEN2(dev) ? 16 : 8) : 1;
+	switch (fb_format_modifier) {
+	case DRM_FORMAT_MOD_NONE:
+		tile_height = 1;
+		break;
+	case I915_FORMAT_MOD_X_TILED:
+		tile_height = IS_GEN2(dev) ? 16 : 8;
+		break;
+	case I915_FORMAT_MOD_Y_TILED:
+		tile_height = 32;
+		break;
+	case I915_FORMAT_MOD_Yf_TILED:
+		bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
+		switch (bits_per_pixel) {
+		default:
+		case 8:
+			tile_height = 64;
+			break;
+		case 16:
+		case 32:
+			tile_height = 32;
+			break;
+		case 64:
+			tile_height = 16;
+			break;
+		case 128:
+			WARN_ONCE(1,
+				  "128-bit pixels are not supported for display!");
+			tile_height = 16;
+			break;
+		}
+		break;
+	default:
+		MISSING_CASE(fb_format_modifier);
+		tile_height = 1;
+		break;
+	}
 
 	return ALIGN(height, tile_height);
 }
-- 
2.3.0

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

[PATCH 4/8] drm/i915/skl: Teach pin_and_fence_fb_obj() about Y tiling constraints

[Tvrtko Ursulin]

From: Damien Lespiau <damien.lespiau@intel.com>

1Mb!

v2: Rebased for addfb2 interface. (Tvrtko Ursulin)
v3: Rebased for fb modifier changes. (Tvrtko Ursulin)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 004e980..f262515 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2270,8 +2270,12 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 		}
 		break;
 	case I915_FORMAT_MOD_Y_TILED:
-		WARN(1, "Y tiled bo slipped through, driver bug!\n");
-		return -EINVAL;
+	case I915_FORMAT_MOD_Yf_TILED:
+		if (WARN_ONCE(INTEL_INFO(dev)->gen < 9,
+			  "Y tiling bo slipped through, driver bug!\n"))
+			return -EINVAL;
+		alignment = 1 * 1024 * 1024;
+		break;
 	default:
 		MISSING_CASE(fb->modifier[0]);
 		return -EINVAL;
-- 
2.3.0

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

[PATCH 5/8] drm/i915/skl: Adjust get_plane_config() to support Yb/Yf tiling

[Tvrtko Ursulin]

From: Damien Lespiau <damien.lespiau@intel.com>

v2: Rebased for addfb2 interface and consolidated a bit. (Tvrtko Ursulin)
v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
v4: Use intel_fb_stride_alignment instead of open coding. (Damien Lespiau)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v3)
---
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f262515..626e6538 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7732,7 +7732,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 val, base, offset, stride_mult;
+	u32 val, base, offset, stride_mult, tiling;
 	int pipe = crtc->pipe;
 	int fourcc, pixel_format;
 	int aligned_height;
@@ -7751,11 +7751,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	if (!(val & PLANE_CTL_ENABLE))
 		goto error;
 
-	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,
 				      val & PLANE_CTL_ORDER_RGBX,
@@ -7763,6 +7758,26 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	fb->pixel_format = fourcc;
 	fb->bits_per_pixel = drm_format_plane_cpp(fourcc, 0) * 8;
 
+	tiling = val & PLANE_CTL_TILED_MASK;
+	switch (tiling) {
+	case PLANE_CTL_TILED_LINEAR:
+		fb->modifier[0] = DRM_FORMAT_MOD_NONE;
+		break;
+	case PLANE_CTL_TILED_X:
+		plane_config->tiling = I915_TILING_X;
+		fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
+		break;
+	case PLANE_CTL_TILED_Y:
+		fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
+		break;
+	case PLANE_CTL_TILED_YF:
+		fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
+		break;
+	default:
+		MISSING_CASE(tiling);
+		goto error;
+	}
+
 	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
 	plane_config->base = base;
 
@@ -7773,17 +7788,8 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	fb->width = ((val >> 0) & 0x1fff) + 1;
 
 	val = I915_READ(PLANE_STRIDE(pipe, 0));
-	switch (plane_config->tiling) {
-	case I915_TILING_NONE:
-		stride_mult = 64;
-		break;
-	case I915_TILING_X:
-		stride_mult = 512;
-		break;
-	default:
-		MISSING_CASE(plane_config->tiling);
-		goto error;
-	}
+	stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
+						fb->pixel_format);
 	fb->pitches[0] = (val & 0x3ff) * stride_mult;
 
 	aligned_height = intel_fb_align_height(dev, fb->height,
-- 
2.3.0

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

[PATCH 6/8] drm/i915/skl: Updated watermark programming

[Tvrtko Ursulin]

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

Recent BSpect updates have changed the watermark calculation to avoid
display flickering in some cases.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f7c9938..626c3c2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2595,7 +2595,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
 	if (latency == 0)
 		return UINT_MAX;
 
-	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel;
+	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel / 512;
 	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
 
 	return ret;
@@ -2605,15 +2605,18 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
 			       uint32_t latency)
 {
-	uint32_t ret, plane_bytes_per_line, wm_intermediate_val;
+	uint32_t ret;
+	uint32_t plane_bytes_per_line, plane_blocks_per_line;
+	uint32_t wm_intermediate_val;
 
 	if (latency == 0)
 		return UINT_MAX;
 
 	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
+	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 	wm_intermediate_val = latency * pixel_rate;
 	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
-				plane_bytes_per_line;
+				plane_blocks_per_line;
 
 	return ret;
 }
@@ -2693,39 +2696,47 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 	}
 }
 
-static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
+static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
+				 struct skl_pipe_wm_parameters *p,
 				 struct intel_plane_wm_parameters *p_params,
 				 uint16_t ddb_allocation,
-				 uint32_t mem_value,
+				 int level,
 				 uint16_t *out_blocks, /* out */
 				 uint8_t *out_lines /* out */)
 {
-	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
-	uint32_t result_bytes;
+	uint32_t latency = dev_priv->wm.skl_latency[level];
+	uint32_t method1, method2;
+	uint32_t plane_bytes_per_line, plane_blocks_per_line;
+	uint32_t res_blocks, res_lines;
+	uint32_t result_blocks;
 
-	if (mem_value == 0 || !p->active || !p_params->enabled)
+	if (latency == 0 || !p->active || !p_params->enabled)
 		return false;
 
 	method1 = skl_wm_method1(p->pixel_rate,
 				 p_params->bytes_per_pixel,
-				 mem_value);
+				 latency);
 	method2 = skl_wm_method2(p->pixel_rate,
 				 p->pipe_htotal,
 				 p_params->horiz_pixels,
 				 p_params->bytes_per_pixel,
-				 mem_value);
+				 latency);
 
 	plane_bytes_per_line = p_params->horiz_pixels *
 					p_params->bytes_per_pixel;
+	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 
 	/* For now xtile and linear */
-	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
-		result_bytes = min(method1, method2);
+	if ((ddb_allocation / plane_blocks_per_line) >= 1)
+		result_blocks = min(method1, method2);
 	else
-		result_bytes = method1;
+		result_blocks = method1;
+
+	res_blocks = result_blocks + 1;
+	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
 
-	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
-	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
+	if (level >=1 && level <= 7)
+		res_blocks++;
 
 	if (res_blocks > ddb_allocation || res_lines > 31)
 		return false;
@@ -2744,23 +2755,24 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 				 int num_planes,
 				 struct skl_wm_level *result)
 {
-	uint16_t latency = dev_priv->wm.skl_latency[level];
 	uint16_t ddb_blocks;
 	int i;
 
 	for (i = 0; i < num_planes; i++) {
 		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
-		result->plane_en[i] = skl_compute_plane_wm(p, &p->plane[i],
+		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
+						p, &p->plane[i],
 						ddb_blocks,
-						latency,
+						level,
 						&result->plane_res_b[i],
 						&result->plane_res_l[i]);
 	}
 
 	ddb_blocks = skl_ddb_entry_size(&ddb->cursor[pipe]);
-	result->cursor_en = skl_compute_plane_wm(p, &p->cursor, ddb_blocks,
-						 latency, &result->cursor_res_b,
+	result->cursor_en = skl_compute_plane_wm(dev_priv, p, &p->cursor,
+						 ddb_blocks, level,
+						 &result->cursor_res_b,
 						 &result->cursor_res_l);
 }
 
-- 
2.3.0

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

[PATCH 7/8] drm/i915/skl: Update watermarks for Y tiling

[Tvrtko Ursulin]

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

Display watermarks need different programming for different tiling
modes.

Set the relevant flag so this happens during the plane commit and
add relevant data into a structure made available to the watermark
computation code.

v2: Pass in tiling info to sprite plane updates as well.
v3: Rebased for plane handling changes.
v4: Handle fb == NULL when plane is disabled.
v5: Refactored for addfb2 interface.
v6: Refactored for fb modifier changes.
v7: Updated for atomic commit by only updating watermarks when tiling changes.
v8: BSpec watermark calculation updates.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Acked-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  6 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 56 ++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
 4 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 626e6538..1d50934 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11994,6 +11994,12 @@ intel_check_primary_plane(struct drm_plane *plane,
 			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 
 		intel_crtc->atomic.update_fbc = true;
+
+		/* Update watermarks on tiling changes. */
+		if (!plane->state->fb || !state->base.fb ||
+		    plane->state->fb->modifier[0] !=
+		    state->base.fb->modifier[0])
+			intel_crtc->atomic.update_wm = true;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 399d2b2..b124548 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
 	uint8_t bytes_per_pixel;
 	bool enabled;
 	bool scaled;
+	u64 tiling;
 };
 
 struct intel_plane {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 626c3c2..e0d6ebc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2603,7 +2603,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
 
 static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
-			       uint32_t latency)
+			       uint64_t tiling, uint32_t latency)
 {
 	uint32_t ret;
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
@@ -2613,7 +2613,16 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 		return UINT_MAX;
 
 	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
-	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+
+	if (tiling == I915_FORMAT_MOD_Y_TILED ||
+	    tiling == I915_FORMAT_MOD_Yf_TILED) {
+		plane_bytes_per_line *= 4;
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+		plane_blocks_per_line /= 4;
+	} else {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+	}
+
 	wm_intermediate_val = latency * pixel_rate;
 	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
 				plane_blocks_per_line;
@@ -2665,6 +2674,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_plane *plane;
+	struct drm_framebuffer *fb;
 	int i = 1; /* Index for sprite planes start */
 
 	p->active = intel_crtc_active(crtc);
@@ -2680,6 +2690,14 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 			crtc->primary->fb->bits_per_pixel / 8;
 		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
 		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
+		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
+		fb = crtc->primary->fb;
+		/*
+		 * Framebuffer can be NULL on plane disable, but it does not
+		 * matter for watermarks if we assume no tiling in that case.
+		 */
+		if (fb)
+			p->plane[0].tiling = fb->modifier[0];
 
 		p->cursor.enabled = true;
 		p->cursor.bytes_per_pixel = 4;
@@ -2709,6 +2727,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
 	uint32_t res_blocks, res_lines;
 	uint32_t result_blocks;
+	uint32_t y_tile_minimum;
 
 	if (latency == 0 || !p->active || !p_params->enabled)
 		return false;
@@ -2720,23 +2739,34 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				 p->pipe_htotal,
 				 p_params->horiz_pixels,
 				 p_params->bytes_per_pixel,
+				 p_params->tiling,
 				 latency);
 
 	plane_bytes_per_line = p_params->horiz_pixels *
 					p_params->bytes_per_pixel;
 	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 
-	/* For now xtile and linear */
-	if ((ddb_allocation / plane_blocks_per_line) >= 1)
-		result_blocks = min(method1, method2);
-	else
-		result_blocks = method1;
+	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
+	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
+		y_tile_minimum = plane_blocks_per_line * 4;
+		result_blocks = max(method2, y_tile_minimum);
+	} else {
+		if ((ddb_allocation / plane_blocks_per_line) >= 1)
+			result_blocks = min(method1, method2);
+		else
+			result_blocks = method1;
+	}
 
 	res_blocks = result_blocks + 1;
 	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
 
-	if (level >=1 && level <= 7)
-		res_blocks++;
+	if (level >=1 && level <= 7) {
+		if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
+		    p_params->tiling == I915_FORMAT_MOD_Yf_TILED)
+			res_lines += 4;
+		else
+			res_blocks++;
+	}
 
 	if (res_blocks > ddb_allocation || res_lines > 31)
 		return false;
@@ -3165,12 +3195,20 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 		     int pixel_size, bool enabled, bool scaled)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_framebuffer *fb = plane->fb;
 
 	intel_plane->wm.enabled = enabled;
 	intel_plane->wm.scaled = scaled;
 	intel_plane->wm.horiz_pixels = sprite_width;
 	intel_plane->wm.vert_pixels = sprite_height;
 	intel_plane->wm.bytes_per_pixel = pixel_size;
+	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
+	/*
+	 * Framebuffer can be NULL on plane disable, but it does not
+	 * matter for watermarks if we assume no tiling in that case.
+	 */
+	if (fb)
+		intel_plane->wm.tiling = fb->modifier[0];
 
 	skl_update_wm(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b7103bd..29ec206 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1256,6 +1256,12 @@ finish:
 
 		if (!intel_crtc->primary_enabled && !state->hides_primary)
 			intel_crtc->atomic.post_enable_primary = true;
+
+		/* Update watermarks on tiling changes. */
+		if (!plane->state->fb || !state->base.fb ||
+		    plane->state->fb->modifier[0] !=
+		    state->base.fb->modifier[0])
+			intel_crtc->atomic.update_wm = true;
 	}
 
 	return 0;
-- 
2.3.0

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

[PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Tvrtko Ursulin]

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

By this patch all underlying bits have been implemented and this
patch actually enables the feature.

v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
---
 drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1d50934..3232ddd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+		/* Passed in modifier sanity checking. */
+		switch (mode_cmd->modifier[0]) {
+		case DRM_FORMAT_MOD_NONE:
+		case I915_FORMAT_MOD_X_TILED:
+		case I915_FORMAT_MOD_Y_TILED:
+		case I915_FORMAT_MOD_Yf_TILED:
+			break;
+		default:
+			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
+				  mode_cmd->modifier[0]);
+			return -EINVAL;
+		}
+
 		/* 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) !=
@@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
 			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
 			return -EINVAL;
 		}
+
+		if (INTEL_INFO(dev)->gen < 9 &&
+		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
+			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
+				  mode_cmd->modifier[0]);
+			return -EINVAL;
+		}
 	} else {
 		if (obj->tiling_mode == I915_TILING_X)
 			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
@@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		}
 	}
 
-	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
-		DRM_DEBUG("hardware does not support tiling Y\n");
-		return -EINVAL;
-	}
-
 	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
 						     mode_cmd->pixel_format);
 	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
-- 
2.3.0

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

Re: [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5826
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              281/281              279/281
ILK                                  308/308              308/308
SNB                 -1              326/326              325/326
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/421              387/421
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_minor-normal-sync      DMESG_WARN(3)PASS(1)      DMESG_WARN(1)PASS(1)
*PNV  igt_gen3_render_tiledx_blits      NO_RESULT(1)PASS(5)      CRASH(1)PASS(1)
 SNB  igt_kms_rotation_crc_primary-rotation      DMESG_WARN(12)PASS(3)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(12)      DMESG_WARN(1)PASS(1)
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 5/8] drm/i915/skl: Adjust get_plane_config() to support Yb/Yf tiling

[Damien Lespiau]

On Wed, Feb 25, 2015 at 04:47:21PM +0000, Tvrtko Ursulin wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> v2: Rebased for addfb2 interface and consolidated a bit. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> v4: Use intel_fb_stride_alignment instead of open coding. (Damien Lespiau)
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v3)

Still looks good to me:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f262515..626e6538 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7732,7 +7732,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 val, base, offset, stride_mult;
> +	u32 val, base, offset, stride_mult, tiling;
>  	int pipe = crtc->pipe;
>  	int fourcc, pixel_format;
>  	int aligned_height;
> @@ -7751,11 +7751,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	if (!(val & PLANE_CTL_ENABLE))
>  		goto error;
>  
> -	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,
>  				      val & PLANE_CTL_ORDER_RGBX,
> @@ -7763,6 +7758,26 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	fb->pixel_format = fourcc;
>  	fb->bits_per_pixel = drm_format_plane_cpp(fourcc, 0) * 8;
>  
> +	tiling = val & PLANE_CTL_TILED_MASK;
> +	switch (tiling) {
> +	case PLANE_CTL_TILED_LINEAR:
> +		fb->modifier[0] = DRM_FORMAT_MOD_NONE;
> +		break;
> +	case PLANE_CTL_TILED_X:
> +		plane_config->tiling = I915_TILING_X;
> +		fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> +		break;
> +	case PLANE_CTL_TILED_Y:
> +		fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
> +		break;
> +	case PLANE_CTL_TILED_YF:
> +		fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
> +		break;
> +	default:
> +		MISSING_CASE(tiling);
> +		goto error;
> +	}
> +
>  	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
>  	plane_config->base = base;
>  
> @@ -7773,17 +7788,8 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	fb->width = ((val >> 0) & 0x1fff) + 1;
>  
>  	val = I915_READ(PLANE_STRIDE(pipe, 0));
> -	switch (plane_config->tiling) {
> -	case I915_TILING_NONE:
> -		stride_mult = 64;
> -		break;
> -	case I915_TILING_X:
> -		stride_mult = 512;
> -		break;
> -	default:
> -		MISSING_CASE(plane_config->tiling);
> -		goto error;
> -	}
> +	stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> +						fb->pixel_format);
>  	fb->pitches[0] = (val & 0x3ff) * stride_mult;
>  
>  	aligned_height = intel_fb_align_height(dev, fb->height,
> -- 
> 2.3.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Daniel Vetter]

On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> By this patch all underlying bits have been implemented and this
> patch actually enables the feature.
> 
> v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1d50934..3232ddd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> +		/* Passed in modifier sanity checking. */
> +		switch (mode_cmd->modifier[0]) {
> +		case DRM_FORMAT_MOD_NONE:
> +		case I915_FORMAT_MOD_X_TILED:
> +		case I915_FORMAT_MOD_Y_TILED:
> +		case I915_FORMAT_MOD_Yf_TILED:
> +			break;
> +		default:
> +			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> +				  mode_cmd->modifier[0]);
> +			return -EINVAL;
> +		}
> +
>  		/* 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) !=
> @@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>  			return -EINVAL;
>  		}
> +
> +		if (INTEL_INFO(dev)->gen < 9 &&
> +		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> +		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> +			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> +				  mode_cmd->modifier[0]);
> +			return -EINVAL;
> +		}
>  	} else {
>  		if (obj->tiling_mode == I915_TILING_X)
>  			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> @@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		}
>  	}
>  
> -	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> -		DRM_DEBUG("hardware does not support tiling Y\n");
> -		return -EINVAL;
> -	}

My idea was actually to put the switch here (reduces one indent level) and
put the per-gen stuff into the switch statement too. I.e.

    	/* Passed in modifier sanity checking. */
    	switch (mode_cmd->modifier[0]) {
    	case I915_FORMAT_MOD_Y_TILED:
    	case I915_FORMAT_MOD_Yf_TILED:
		if (INTEL_INFO(dev)->gen < 9) {
			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
				  mode_cmd->modifier[0]);
			return -EINVAL;

		}
    	case DRM_FORMAT_MOD_NONE:
    	case I915_FORMAT_MOD_X_TILED:
    		break;
    	default:
    		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
    			  mode_cmd->modifier[0]);
    		return -EINVAL;
    	}

That gives us a natural place for extensions later on if we need to add
more special cases.
-Daniel

> -
>  	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
>  						     mode_cmd->pixel_format);
>  	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 6/8] drm/i915/skl: Updated watermark programming

[Damien Lespiau]

On Wed, Feb 25, 2015 at 04:47:22PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Recent BSpect updates have changed the watermark calculation to avoid
> display flickering in some cases.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---

There are really several changes in this patch, it would have been
easier for the review to split them (and that's usually how we are
supposed to split patches).

  - Convert the inner computations to number of blocks
  - W/A to increase the nb of blocks for level 1-7 by 1
  - Move max block test to >= instead of >

Anyway, which the couple of comments below addressd (of which only the
'>=' is a real problem), this is:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

>  drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7c9938..626c3c2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2595,7 +2595,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>  	if (latency == 0)
>  		return UINT_MAX;
>  
> -	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel;
> +	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel / 512;
>  	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
>  
>  	return ret;
> @@ -2605,15 +2605,18 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>  			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>  			       uint32_t latency)
>  {
> -	uint32_t ret, plane_bytes_per_line, wm_intermediate_val;
> +	uint32_t ret;
> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> +	uint32_t wm_intermediate_val;
>  
>  	if (latency == 0)
>  		return UINT_MAX;
>  
>  	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  	wm_intermediate_val = latency * pixel_rate;
>  	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
> -				plane_bytes_per_line;
> +				plane_blocks_per_line;
>  
>  	return ret;
>  }
> @@ -2693,39 +2696,47 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	}
>  }
>  
> -static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
> +static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> +				 struct skl_pipe_wm_parameters *p,
>  				 struct intel_plane_wm_parameters *p_params,
>  				 uint16_t ddb_allocation,
> -				 uint32_t mem_value,
> +				 int level,
>  				 uint16_t *out_blocks, /* out */
>  				 uint8_t *out_lines /* out */)
>  {
> -	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
> -	uint32_t result_bytes;
> +	uint32_t latency = dev_priv->wm.skl_latency[level];
> +	uint32_t method1, method2;
> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> +	uint32_t res_blocks, res_lines;
> +	uint32_t result_blocks;

we now have res_blocks and result_blocks, a bit confusing. Maybe rename
result_blocks to selected_result (which happens to be in blocks)

>  
> -	if (mem_value == 0 || !p->active || !p_params->enabled)
> +	if (latency == 0 || !p->active || !p_params->enabled)
>  		return false;
>  
>  	method1 = skl_wm_method1(p->pixel_rate,
>  				 p_params->bytes_per_pixel,
> -				 mem_value);
> +				 latency);
>  	method2 = skl_wm_method2(p->pixel_rate,
>  				 p->pipe_htotal,
>  				 p_params->horiz_pixels,
>  				 p_params->bytes_per_pixel,
> -				 mem_value);
> +				 latency);
>  
>  	plane_bytes_per_line = p_params->horiz_pixels *
>  					p_params->bytes_per_pixel;
> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  
>  	/* For now xtile and linear */
> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> -		result_bytes = min(method1, method2);
> +	if ((ddb_allocation / plane_blocks_per_line) >= 1)
> +		result_blocks = min(method1, method2);
>  	else
> -		result_bytes = method1;
> +		result_blocks = method1;
> +
> +	res_blocks = result_blocks + 1;
> +	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
>  
> -	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
> -	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
> +	if (level >=1 && level <= 7)

a space is missing before the 1 :)

> +		res_blocks++;
>  
>  	if (res_blocks > ddb_allocation || res_lines > 31)

res_blocks >= ddb_allocation

>  		return false;
> @@ -2744,23 +2755,24 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  				 int num_planes,
>  				 struct skl_wm_level *result)
>  {
> -	uint16_t latency = dev_priv->wm.skl_latency[level];
>  	uint16_t ddb_blocks;
>  	int i;
>  
>  	for (i = 0; i < num_planes; i++) {
>  		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>  
> -		result->plane_en[i] = skl_compute_plane_wm(p, &p->plane[i],
> +		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
> +						p, &p->plane[i],
>  						ddb_blocks,
> -						latency,
> +						level,
>  						&result->plane_res_b[i],
>  						&result->plane_res_l[i]);
>  	}
>  
>  	ddb_blocks = skl_ddb_entry_size(&ddb->cursor[pipe]);
> -	result->cursor_en = skl_compute_plane_wm(p, &p->cursor, ddb_blocks,
> -						 latency, &result->cursor_res_b,
> +	result->cursor_en = skl_compute_plane_wm(dev_priv, p, &p->cursor,
> +						 ddb_blocks, level,
> +						 &result->cursor_res_b,
>  						 &result->cursor_res_l);
>  }
>  
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/skl: Update watermarks for Y tiling

[Damien Lespiau]

On Wed, Feb 25, 2015 at 04:47:23PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Display watermarks need different programming for different tiling
> modes.
> 
> Set the relevant flag so this happens during the plane commit and
> add relevant data into a structure made available to the watermark
> computation code.
> 
> v2: Pass in tiling info to sprite plane updates as well.
> v3: Rebased for plane handling changes.
> v4: Handle fb == NULL when plane is disabled.
> v5: Refactored for addfb2 interface.
> v6: Refactored for fb modifier changes.
> v7: Updated for atomic commit by only updating watermarks when tiling changes.
> v8: BSpec watermark calculation updates.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Acked-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |  6 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 56 ++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
>  4 files changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 626e6538..1d50934 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,6 +11994,12 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>  
>  		intel_crtc->atomic.update_fbc = true;
> +
> +		/* Update watermarks on tiling changes. */
> +		if (!plane->state->fb || !state->base.fb ||
> +		    plane->state->fb->modifier[0] !=
> +		    state->base.fb->modifier[0])
> +			intel_crtc->atomic.update_wm = true;
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 399d2b2..b124548 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
>  	uint8_t bytes_per_pixel;
>  	bool enabled;
>  	bool scaled;
> +	u64 tiling;
>  };
>  
>  struct intel_plane {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 626c3c2..e0d6ebc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2603,7 +2603,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>  
>  static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>  			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
> -			       uint32_t latency)
> +			       uint64_t tiling, uint32_t latency)
>  {

Hum, does this compile? I'm seeing an extra argument to skl_wm_method2()
but no update at the calling site?

>  	uint32_t ret;
>  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> @@ -2613,7 +2613,16 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>  		return UINT_MAX;
>  
>  	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
> -	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +
> +	if (tiling == I915_FORMAT_MOD_Y_TILED ||
> +	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> +		plane_bytes_per_line *= 4;
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +		plane_blocks_per_line /= 4;
> +	} else {
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +	}
> +
>  	wm_intermediate_val = latency * pixel_rate;
>  	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>  				plane_blocks_per_line;
> @@ -2665,6 +2674,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_plane *plane;
> +	struct drm_framebuffer *fb;
>  	int i = 1; /* Index for sprite planes start */
>  
>  	p->active = intel_crtc_active(crtc);
> @@ -2680,6 +2690,14 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  			crtc->primary->fb->bits_per_pixel / 8;
>  		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
>  		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> +		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> +		fb = crtc->primary->fb;
> +		/*
> +		 * Framebuffer can be NULL on plane disable, but it does not
> +		 * matter for watermarks if we assume no tiling in that case.
> +		 */
> +		if (fb)
> +			p->plane[0].tiling = fb->modifier[0];
>  
>  		p->cursor.enabled = true;
>  		p->cursor.bytes_per_pixel = 4;
> @@ -2709,6 +2727,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>  	uint32_t res_blocks, res_lines;
>  	uint32_t result_blocks;
> +	uint32_t y_tile_minimum;

The scope of the y_tile_minimum variable could be restricted further,
but well...

>  	if (latency == 0 || !p->active || !p_params->enabled)
>  		return false;
> @@ -2720,23 +2739,34 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  				 p->pipe_htotal,
>  				 p_params->horiz_pixels,
>  				 p_params->bytes_per_pixel,
> +				 p_params->tiling,
>  				 latency);
>  
>  	plane_bytes_per_line = p_params->horiz_pixels *
>  					p_params->bytes_per_pixel;
>  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  
> -	/* For now xtile and linear */
> -	if ((ddb_allocation / plane_blocks_per_line) >= 1)
> -		result_blocks = min(method1, method2);
> -	else
> -		result_blocks = method1;
> +	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> +	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> +		y_tile_minimum = plane_blocks_per_line * 4;
> +		result_blocks = max(method2, y_tile_minimum);
> +	} else {
> +		if ((ddb_allocation / plane_blocks_per_line) >= 1)
> +			result_blocks = min(method1, method2);
> +		else
> +			result_blocks = method1;
> +	}
>  
>  	res_blocks = result_blocks + 1;
>  	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
>  
> -	if (level >=1 && level <= 7)
> -		res_blocks++;
> +	if (level >=1 && level <= 7) {
> +		if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> +		    p_params->tiling == I915_FORMAT_MOD_Yf_TILED)
> +			res_lines += 4;
> +		else
> +			res_blocks++;
> +	}
>  
>  	if (res_blocks > ddb_allocation || res_lines > 31)
>  		return false;
> @@ -3165,12 +3195,20 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
>  		     int pixel_size, bool enabled, bool scaled)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_framebuffer *fb = plane->fb;
>  
>  	intel_plane->wm.enabled = enabled;
>  	intel_plane->wm.scaled = scaled;
>  	intel_plane->wm.horiz_pixels = sprite_width;
>  	intel_plane->wm.vert_pixels = sprite_height;
>  	intel_plane->wm.bytes_per_pixel = pixel_size;
> +	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
> +	/*
> +	 * Framebuffer can be NULL on plane disable, but it does not
> +	 * matter for watermarks if we assume no tiling in that case.
> +	 */
> +	if (fb)
> +		intel_plane->wm.tiling = fb->modifier[0];
>  
>  	skl_update_wm(crtc);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b7103bd..29ec206 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1256,6 +1256,12 @@ finish:
>  
>  		if (!intel_crtc->primary_enabled && !state->hides_primary)
>  			intel_crtc->atomic.post_enable_primary = true;
> +
> +		/* Update watermarks on tiling changes. */
> +		if (!plane->state->fb || !state->base.fb ||
> +		    plane->state->fb->modifier[0] !=
> +		    state->base.fb->modifier[0])
> +			intel_crtc->atomic.update_wm = true;
>  	}
>  
>  	return 0;
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/8] drm/i915/skl: Updated watermark programming

[Tvrtko Ursulin]

On 02/26/2015 04:45 PM, Damien Lespiau wrote:
> On Wed, Feb 25, 2015 at 04:47:22PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Recent BSpect updates have changed the watermark calculation to avoid
>> display flickering in some cases.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>
> There are really several changes in this patch, it would have been
> easier for the review to split them (and that's usually how we are
> supposed to split patches).

 From my point of view - this patch changes the computation from what it 
was documented two weeks ago, to what was documented one week ago. I 
don't see that it would be natural to be so inventive to start splitting 
that.

>    - Convert the inner computations to number of blocks
>    - W/A to increase the nb of blocks for level 1-7 by 1
>    - Move max block test to >= instead of >
>
> Anyway, which the couple of comments below addressd (of which only the
> '>=' is a real problem), this is:
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Will fix up the details and tag it then, thanks!

>>   drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++++++++++----------------
>>   1 file changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index f7c9938..626c3c2 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2595,7 +2595,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>>   	if (latency == 0)
>>   		return UINT_MAX;
>>
>> -	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel;
>> +	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel / 512;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
>>
>>   	return ret;
>> @@ -2605,15 +2605,18 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>>   			       uint32_t latency)
>>   {
>> -	uint32_t ret, plane_bytes_per_line, wm_intermediate_val;
>> +	uint32_t ret;
>> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> +	uint32_t wm_intermediate_val;
>>
>>   	if (latency == 0)
>>   		return UINT_MAX;
>>
>>   	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
>> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>>   	wm_intermediate_val = latency * pixel_rate;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>> -				plane_bytes_per_line;
>> +				plane_blocks_per_line;
>>
>>   	return ret;
>>   }
>> @@ -2693,39 +2696,47 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   	}
>>   }
>>
>> -static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>> +static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>> +				 struct skl_pipe_wm_parameters *p,
>>   				 struct intel_plane_wm_parameters *p_params,
>>   				 uint16_t ddb_allocation,
>> -				 uint32_t mem_value,
>> +				 int level,
>>   				 uint16_t *out_blocks, /* out */
>>   				 uint8_t *out_lines /* out */)
>>   {
>> -	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
>> -	uint32_t result_bytes;
>> +	uint32_t latency = dev_priv->wm.skl_latency[level];
>> +	uint32_t method1, method2;
>> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> +	uint32_t res_blocks, res_lines;
>> +	uint32_t result_blocks;
>
> we now have res_blocks and result_blocks, a bit confusing. Maybe rename
> result_blocks to selected_result (which happens to be in blocks)

Will change.

>>
>> -	if (mem_value == 0 || !p->active || !p_params->enabled)
>> +	if (latency == 0 || !p->active || !p_params->enabled)
>>   		return false;
>>
>>   	method1 = skl_wm_method1(p->pixel_rate,
>>   				 p_params->bytes_per_pixel,
>> -				 mem_value);
>> +				 latency);
>>   	method2 = skl_wm_method2(p->pixel_rate,
>>   				 p->pipe_htotal,
>>   				 p_params->horiz_pixels,
>>   				 p_params->bytes_per_pixel,
>> -				 mem_value);
>> +				 latency);
>>
>>   	plane_bytes_per_line = p_params->horiz_pixels *
>>   					p_params->bytes_per_pixel;
>> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>>
>>   	/* For now xtile and linear */
>> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
>> -		result_bytes = min(method1, method2);
>> +	if ((ddb_allocation / plane_blocks_per_line) >= 1)
>> +		result_blocks = min(method1, method2);
>>   	else
>> -		result_bytes = method1;
>> +		result_blocks = method1;
>> +
>> +	res_blocks = result_blocks + 1;
>> +	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
>>
>> -	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>> -	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
>> +	if (level >=1 && level <= 7)
>
> a space is missing before the 1 :)

Weird, ok. :)

>> +		res_blocks++;
>>
>>   	if (res_blocks > ddb_allocation || res_lines > 31)
>
> res_blocks >= ddb_allocation

Well spotted.

Regards,

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

Re: [PATCH 7/8] drm/i915/skl: Update watermarks for Y tiling

[Tvrtko Ursulin]

On 02/26/2015 04:59 PM, Damien Lespiau wrote:
> On Wed, Feb 25, 2015 at 04:47:23PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Display watermarks need different programming for different tiling
>> modes.
>>
>> Set the relevant flag so this happens during the plane commit and
>> add relevant data into a structure made available to the watermark
>> computation code.
>>
>> v2: Pass in tiling info to sprite plane updates as well.
>> v3: Rebased for plane handling changes.
>> v4: Handle fb == NULL when plane is disabled.
>> v5: Refactored for addfb2 interface.
>> v6: Refactored for fb modifier changes.
>> v7: Updated for atomic commit by only updating watermarks when tiling changes.
>> v8: BSpec watermark calculation updates.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Acked-by: Matt Roper <matthew.d.roper@intel.com>
>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |  6 ++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c      | 56 ++++++++++++++++++++++++++++++------
>>   drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
>>   4 files changed, 60 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 626e6538..1d50934 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11994,6 +11994,12 @@ intel_check_primary_plane(struct drm_plane *plane,
>>   			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>
>>   		intel_crtc->atomic.update_fbc = true;
>> +
>> +		/* Update watermarks on tiling changes. */
>> +		if (!plane->state->fb || !state->base.fb ||
>> +		    plane->state->fb->modifier[0] !=
>> +		    state->base.fb->modifier[0])
>> +			intel_crtc->atomic.update_wm = true;
>>   	}
>>
>>   	return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 399d2b2..b124548 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
>>   	uint8_t bytes_per_pixel;
>>   	bool enabled;
>>   	bool scaled;
>> +	u64 tiling;
>>   };
>>
>>   struct intel_plane {
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 626c3c2..e0d6ebc 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2603,7 +2603,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>>
>>   static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>> -			       uint32_t latency)
>> +			       uint64_t tiling, uint32_t latency)
>>   {
>
> Hum, does this compile? I'm seeing an extra argument to skl_wm_method2()
> but no update at the calling site?

Not only that, but it even works! :) (Extra argument is there, you must 
have missed it!)

>>   	uint32_t ret;
>>   	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> @@ -2613,7 +2613,16 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   		return UINT_MAX;
>>
>>   	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
>> -	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +
>> +	if (tiling == I915_FORMAT_MOD_Y_TILED ||
>> +	    tiling == I915_FORMAT_MOD_Yf_TILED) {
>> +		plane_bytes_per_line *= 4;
>> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +		plane_blocks_per_line /= 4;
>> +	} else {
>> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +	}
>> +
>>   	wm_intermediate_val = latency * pixel_rate;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>>   				plane_blocks_per_line;
>> @@ -2665,6 +2674,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>   	enum pipe pipe = intel_crtc->pipe;
>>   	struct drm_plane *plane;
>> +	struct drm_framebuffer *fb;
>>   	int i = 1; /* Index for sprite planes start */
>>
>>   	p->active = intel_crtc_active(crtc);
>> @@ -2680,6 +2690,14 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   			crtc->primary->fb->bits_per_pixel / 8;
>>   		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
>>   		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
>> +		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>> +		fb = crtc->primary->fb;
>> +		/*
>> +		 * Framebuffer can be NULL on plane disable, but it does not
>> +		 * matter for watermarks if we assume no tiling in that case.
>> +		 */
>> +		if (fb)
>> +			p->plane[0].tiling = fb->modifier[0];
>>
>>   		p->cursor.enabled = true;
>>   		p->cursor.bytes_per_pixel = 4;
>> @@ -2709,6 +2727,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>   	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>>   	uint32_t res_blocks, res_lines;
>>   	uint32_t result_blocks;
>> +	uint32_t y_tile_minimum;
>
> The scope of the y_tile_minimum variable could be restricted further,
> but well...

Okay, okay, I'm in the old C standard for kernel still.

Regards,

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

Re: [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Tvrtko Ursulin]

On 02/26/2015 04:44 PM, Daniel Vetter wrote:
> On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> By this patch all underlying bits have been implemented and this
>> patch actually enables the feature.
>>
>> v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1d50934..3232ddd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>>   	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>> +		/* Passed in modifier sanity checking. */
>> +		switch (mode_cmd->modifier[0]) {
>> +		case DRM_FORMAT_MOD_NONE:
>> +		case I915_FORMAT_MOD_X_TILED:
>> +		case I915_FORMAT_MOD_Y_TILED:
>> +		case I915_FORMAT_MOD_Yf_TILED:
>> +			break;
>> +		default:
>> +			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
>> +				  mode_cmd->modifier[0]);
>> +			return -EINVAL;
>> +		}
>> +
>>   		/* 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) !=
>> @@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>>   			return -EINVAL;
>>   		}
>> +
>> +		if (INTEL_INFO(dev)->gen < 9 &&
>> +		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>> +		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>> +			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
>> +				  mode_cmd->modifier[0]);
>> +			return -EINVAL;
>> +		}
>>   	} else {
>>   		if (obj->tiling_mode == I915_TILING_X)
>>   			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
>> @@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   		}
>>   	}
>>
>> -	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
>> -		DRM_DEBUG("hardware does not support tiling Y\n");
>> -		return -EINVAL;
>> -	}
>
> My idea was actually to put the switch here (reduces one indent level) and
> put the per-gen stuff into the switch statement too. I.e.
>
>      	/* Passed in modifier sanity checking. */
>      	switch (mode_cmd->modifier[0]) {
>      	case I915_FORMAT_MOD_Y_TILED:
>      	case I915_FORMAT_MOD_Yf_TILED:
> 		if (INTEL_INFO(dev)->gen < 9) {
> 			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> 				  mode_cmd->modifier[0]);
> 			return -EINVAL;
>
> 		}
>      	case DRM_FORMAT_MOD_NONE:
>      	case I915_FORMAT_MOD_X_TILED:
>      		break;
>      	default:
>      		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
>      			  mode_cmd->modifier[0]);
>      		return -EINVAL;
>      	}
>
> That gives us a natural place for extensions later on if we need to add
> more special cases.

Yes I agree this patch being only on v2 was way disproportionate 
compared to some others from this series. ;)

Regards,

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

Re: [PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Daniel Vetter]

On Fri, Feb 27, 2015 at 09:45:27AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/26/2015 04:44 PM, Daniel Vetter wrote:
> >On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>By this patch all underlying bits have been implemented and this
> >>patch actually enables the feature.
> >>
> >>v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
> >>  1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 1d50934..3232ddd 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>
> >>  	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>+		/* Passed in modifier sanity checking. */
> >>+		switch (mode_cmd->modifier[0]) {
> >>+		case DRM_FORMAT_MOD_NONE:
> >>+		case I915_FORMAT_MOD_X_TILED:
> >>+		case I915_FORMAT_MOD_Y_TILED:
> >>+		case I915_FORMAT_MOD_Yf_TILED:
> >>+			break;
> >>+		default:
> >>+			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> >>+				  mode_cmd->modifier[0]);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>  		/* 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) !=
> >>@@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> >>  			return -EINVAL;
> >>  		}
> >>+
> >>+		if (INTEL_INFO(dev)->gen < 9 &&
> >>+		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>+		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>+			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> >>+				  mode_cmd->modifier[0]);
> >>+			return -EINVAL;
> >>+		}
> >>  	} else {
> >>  		if (obj->tiling_mode == I915_TILING_X)
> >>  			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> >>@@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  		}
> >>  	}
> >>
> >>-	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> >>-		DRM_DEBUG("hardware does not support tiling Y\n");
> >>-		return -EINVAL;
> >>-	}
> >
> >My idea was actually to put the switch here (reduces one indent level) and
> >put the per-gen stuff into the switch statement too. I.e.
> >
> >     	/* Passed in modifier sanity checking. */
> >     	switch (mode_cmd->modifier[0]) {
> >     	case I915_FORMAT_MOD_Y_TILED:
> >     	case I915_FORMAT_MOD_Yf_TILED:
> >		if (INTEL_INFO(dev)->gen < 9) {
> >			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> >				  mode_cmd->modifier[0]);
> >			return -EINVAL;
> >
> >		}
> >     	case DRM_FORMAT_MOD_NONE:
> >     	case I915_FORMAT_MOD_X_TILED:
> >     		break;
> >     	default:
> >     		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> >     			  mode_cmd->modifier[0]);
> >     		return -EINVAL;
> >     	}
> >
> >That gives us a natural place for extensions later on if we need to add
> >more special cases.
> 
> Yes I agree this patch being only on v2 was way disproportionate compared to
> some others from this series. ;)

Well I've obviously failed to explain properly in my review to v1 to avoid
a v3 ;-)
-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 7/8] drm/i915/skl: Update watermarks for Y tiling

[Damien Lespiau]

On Fri, Feb 27, 2015 at 09:39:47AM +0000, Tvrtko Ursulin wrote:
> >Hum, does this compile? I'm seeing an extra argument to skl_wm_method2()
> >but no update at the calling site?
> 
> Not only that, but it even works! :) (Extra argument is there, you must have
> missed it!)

Ooops, I see it now:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

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

[PATCH 8/8] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Tvrtko Ursulin]

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

By this patch all underlying bits have been implemented and this
patch actually enables the feature.

v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
v3: Rearrange validation checks per code review comments. (Daniel Vetter)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4056fad..1dd0326 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12825,8 +12825,21 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		}
 	}
 
-	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
-		DRM_DEBUG("hardware does not support tiling Y\n");
+	/* Passed in modifier sanity checking. */
+	switch (mode_cmd->modifier[0]) {
+	case I915_FORMAT_MOD_Y_TILED:
+	case I915_FORMAT_MOD_Yf_TILED:
+		if (INTEL_INFO(dev)->gen < 9) {
+			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
+				  mode_cmd->modifier[0]);
+			return -EINVAL;
+		}
+	case DRM_FORMAT_MOD_NONE:
+	case I915_FORMAT_MOD_X_TILED:
+		break;
+	default:
+		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
+				mode_cmd->modifier[0]);
 		return -EINVAL;
 	}
 
-- 
2.3.0

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