[PATCH 0/7] Skylake Y tiled scanout

[PATCH 0/7] 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.

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 (3):
  drm/i915/skl: Add new displayable tiling formats
  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 | 218 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_pm.c      |  33 +++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  24 +++-
 include/uapi/drm/drm_fourcc.h        |  15 +++
 5 files changed, 222 insertions(+), 71 deletions(-)

-- 
2.3.0

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

[PATCH 1/7] 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/7] 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ä)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e70748..a523d84 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2728,6 +2728,34 @@ 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:
+		return 512;
+	case I915_FORMAT_MOD_Y_TILED:
+		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 +2763,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 +2809,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 +2845,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 +12685,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 +12747,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/7] 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)

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 | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a523d84..4f0033a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2195,9 +2195,36 @@ 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 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:
+		case 128:
+			tile_height = 16;
+			break;
+		}
+		break;
+	default:
+		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/7] 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 4f0033a..358a97e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2262,8 +2262,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/7] 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)

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 | 45 ++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 358a97e..c622b11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7718,7 +7718,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;
@@ -7737,11 +7737,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,
@@ -7749,6 +7744,33 @@ 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;
+		stride_mult = 64;
+		break;
+	case PLANE_CTL_TILED_X:
+		plane_config->tiling = I915_TILING_X;
+		fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
+		stride_mult = 512;
+		break;
+	case PLANE_CTL_TILED_Y:
+		fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
+		stride_mult = 128;
+		break;
+	case PLANE_CTL_TILED_YF:
+		fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
+		if (fb->bits_per_pixel == 8)
+			stride_mult = 64;
+		else
+			stride_mult = 128;
+		break;
+	default:
+		MISSING_CASE(tiling);
+		goto error;
+	}
+
 	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
 	plane_config->base = base;
 
@@ -7759,17 +7781,6 @@ 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;
-	}
 	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/7] 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.

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      | 33 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c622b11..74d4923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11985,6 +11985,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 f7c9938..006e635 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2662,6 +2662,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);
@@ -2677,6 +2678,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;
@@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 {
 	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
 	uint32_t result_bytes;
+	uint32_t y_tile_minimum;
 
 	if (mem_value == 0 || !p->active || !p_params->enabled)
 		return false;
@@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 	plane_bytes_per_line = p_params->horiz_pixels *
 					p_params->bytes_per_pixel;
 
-	/* For now xtile and linear */
-	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
-		result_bytes = min(method1, method2);
-	else
-		result_bytes = method1;
+	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
+	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
+		y_tile_minimum = plane_bytes_per_line * 4;
+		result_bytes = max(method2, y_tile_minimum);
+	} else {
+		if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
+			result_bytes = min(method1, method2);
+		else
+			result_bytes = method1;
+	}
 
 	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
 	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
@@ -3153,12 +3168,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 7/7] 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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 74d4923..f100086 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12781,6 +12781,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;
@@ -12790,11 +12798,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 2/7] drm/i915/skl: Allow scanning out Y and Yf fbs

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:55:56PM +0000, Tvrtko Ursulin wrote:
> 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ä)
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

While Tvrtko had the courtesy of keeping me as the original author, it's
really two authors, so the r-b has some meaning. Anyway:

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

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
>  3 files changed, 95 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e70748..a523d84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2728,6 +2728,34 @@ 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:
> +		return 512;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		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 +2763,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 +2809,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 +2845,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 +12685,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 +12747,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

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

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
> 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)
> 
> 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>

Might want to add a MISSING_CASE() here as well. For the record, the
vertical alignments for Yf are taken from a dodgy looking document found
by chance.

We don't have a 128bpp format supported by display

Now that I think about it, for the horizontal stride, BSpec says:

  - 8 bpp -> 64 bytes
  - * bpp -> 128 bytes

Given that a tile is a page size this would give for the vertical
alignment:

  - 8 bpp -> 64 byes (that's what we have)
  - * -> 32 bytes

So the 64bpp cases look a bit suspicious. Given that one the goals for
this new tiling format is to have tiles as square as possible (in terms
of pixels, not byte size), it'd make sense to have different strides
constraints for the 64bpp format, so it could be BSpec being wrong on
the horizontal stride constraint for 64bpp?

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a523d84..4f0033a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2195,9 +2195,36 @@ 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 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:
> +		case 128:
> +			tile_height = 16;
> +			break;
> +		}
> +		break;
> +	default:
> +		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

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

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:55:59PM +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)
> 
> 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>
> ---

This patch looks like it could reuse the newly introduced
intel_fb_stride_alignment(). Otherwise:

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

>  drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 358a97e..c622b11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7718,7 +7718,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;
> @@ -7737,11 +7737,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,
> @@ -7749,6 +7744,33 @@ 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;
> +		stride_mult = 64;
> +		break;
> +	case PLANE_CTL_TILED_X:
> +		plane_config->tiling = I915_TILING_X;
> +		fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> +		stride_mult = 512;
> +		break;
> +	case PLANE_CTL_TILED_Y:
> +		fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
> +		stride_mult = 128;
> +		break;
> +	case PLANE_CTL_TILED_YF:
> +		fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
> +		if (fb->bits_per_pixel == 8)
> +			stride_mult = 64;
> +		else
> +			stride_mult = 128;
> +		break;
> +	default:
> +		MISSING_CASE(tiling);
> +		goto error;
> +	}
> +
>  	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
>  	plane_config->base = base;
>  
> @@ -7759,17 +7781,6 @@ 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;
> -	}
>  	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 2/7] drm/i915/skl: Allow scanning out Y and Yf fbs

[Ville Syrjälä]

On Mon, Feb 23, 2015 at 03:55:56PM +0000, Tvrtko Ursulin wrote:
> 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ä)
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
>  3 files changed, 95 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e70748..a523d84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2728,6 +2728,34 @@ 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 (gen2)
	return 128;

> +		return 512;
> +	case I915_FORMAT_MOD_Y_TILED:
> +		return 128;

In theory we could check gen2 and HAS_128_BYTE_Y_TILING() here, but
since old platforms didn't do Y tiled scanout anyway that's not really
needed. But maybe toss in a comment about that so that people don't
start wondering why this doesn't care about that stuff?

> +	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 +2763,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 +2809,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 +2845,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 +12685,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 +12747,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);

We seem to alternate between "stride" and "pitch" somewhat randomly.
Should maybe stick to one or other :)

Anyways this looks pretty good to me. With the gen2 X tiled stride
alignment fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] Skylake Y tiled scanout

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:55:54PM +0000, Tvrtko Ursulin wrote:
> 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".

Just a note that we have and more and more code using modifiers[0]
direction and not masking it for the bits it really needs (tiling). This
will mean we need to change it all over the place when adding a new
modifier. Might not be ideal.

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

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

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:56:00PM +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.
> 
> 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      | 33 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c622b11..74d4923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11985,6 +11985,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 f7c9938..006e635 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2662,6 +2662,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);
> @@ -2677,6 +2678,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;
> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  {
>  	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
>  	uint32_t result_bytes;
> +	uint32_t y_tile_minimum;
>  
>  	if (mem_value == 0 || !p->active || !p_params->enabled)
>  		return false;
> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  	plane_bytes_per_line = p_params->horiz_pixels *
>  					p_params->bytes_per_pixel;
>  
> -	/* For now xtile and linear */
> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> -		result_bytes = min(method1, method2);
> -	else
> -		result_bytes = method1;
> +	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> +	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> +		y_tile_minimum = plane_bytes_per_line * 4;
> +		result_bytes = max(method2, y_tile_minimum);
> +	} else {
> +		if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> +			result_bytes = min(method1, method2);
> +		else
> +			result_bytes = method1;
> +	}

While this was what was documented at some point, it's not anymore.
Would need an extra patch before this one to introduce
plane_blocks_per_line and then this one on top.

>  
>  	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>  	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
> @@ -3153,12 +3168,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 7/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Damien Lespiau]

On Mon, Feb 23, 2015 at 03:56:01PM +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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---

Ville noticed that it didn't seem we were properly returning -EINVAL
when userspace sets reserved bits in the modifiers field. Otherwise,
what's here seems to progress in the right direction:

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

-- 
Damien

>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74d4923..f100086 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,6 +12781,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;
> @@ -12790,11 +12798,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Feb 23, 2015 at 03:56:00PM +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.
> 
> 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      | 33 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c622b11..74d4923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11985,6 +11985,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;

Imo this is fragile. We should unconditionally recomupte watermarks imo
and just ellide the hw update if nothing changed. Spending a few cpu
cycles on this per frame shouldn't hurt, and it will avoid duplicated
logic and checks for watermark recomputation splattered all over the code.

I know that it's kinda there already, but still minimal enough to patch
up. But if Matt knows that this will get removed again with the two-stage
wm code then I'm ok with leaving it in, with a FIXME comment added.
-Daniel

>  	}
>  
>  	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 f7c9938..006e635 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2662,6 +2662,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);
> @@ -2677,6 +2678,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;
> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  {
>  	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
>  	uint32_t result_bytes;
> +	uint32_t y_tile_minimum;
>  
>  	if (mem_value == 0 || !p->active || !p_params->enabled)
>  		return false;
> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  	plane_bytes_per_line = p_params->horiz_pixels *
>  					p_params->bytes_per_pixel;
>  
> -	/* For now xtile and linear */
> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> -		result_bytes = min(method1, method2);
> -	else
> -		result_bytes = method1;
> +	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> +	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> +		y_tile_minimum = plane_bytes_per_line * 4;
> +		result_bytes = max(method2, y_tile_minimum);
> +	} else {
> +		if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> +			result_bytes = min(method1, method2);
> +		else
> +			result_bytes = method1;
> +	}
>  
>  	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>  	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
> @@ -3153,12 +3168,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

-- 
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/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation

[Daniel Vetter]

On Mon, Feb 23, 2015 at 03:56:01PM +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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74d4923..f100086 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,6 +12781,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;
> @@ -12790,11 +12798,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;
> -	}

Imo the clearer code would be to add a 
	
	switch (mode_cmd->modifier[0]) {

	}

here and then shovel the platform checks into the supgroups. At least
that's how we tend to roll these since it reduces the risks that a case is
forgotten when the enumeration is extended. That would also have caught
that we don't reject random garbage in the default: case.
-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 7/7] 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: 5814
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  281/281              281/281
ILK                                  305/305              305/305
SNB                 -1              326/326              325/326
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                 -1              421/421              420/421
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_plane_plane-position-hole-pipe-B-plane-1      DMESG_WARN(12)PASS(3)      DMESG_WARN(1)PASS(1)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(2)PASS(1)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(6)      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 6/7] drm/i915/skl: Update watermarks for Y tiling

[Tvrtko Ursulin]

On 02/24/2015 07:26 PM, Damien Lespiau wrote:
> On Mon, Feb 23, 2015 at 03:56:00PM +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.
>>
>> 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      | 33 ++++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
>>   4 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c622b11..74d4923 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11985,6 +11985,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 f7c9938..006e635 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2662,6 +2662,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);
>> @@ -2677,6 +2678,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;
>> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>>   {
>>   	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
>>   	uint32_t result_bytes;
>> +	uint32_t y_tile_minimum;
>>
>>   	if (mem_value == 0 || !p->active || !p_params->enabled)
>>   		return false;
>> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>>   	plane_bytes_per_line = p_params->horiz_pixels *
>>   					p_params->bytes_per_pixel;
>>
>> -	/* For now xtile and linear */
>> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
>> -		result_bytes = min(method1, method2);
>> -	else
>> -		result_bytes = method1;
>> +	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
>> +	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
>> +		y_tile_minimum = plane_bytes_per_line * 4;
>> +		result_bytes = max(method2, y_tile_minimum);
>> +	} else {
>> +		if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
>> +			result_bytes = min(method1, method2);
>> +		else
>> +			result_bytes = method1;
>> +	}
>
> While this was what was documented at some point, it's not anymore.
> Would need an extra patch before this one to introduce
> plane_blocks_per_line and then this one on top.

Don't follow - what is plane_blocks_per_line and where and how would you 
add it before this patch?

Regards,

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

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

[Tvrtko Ursulin]

On 02/24/2015 05:36 PM, Ville Syrjälä wrote:
> On Mon, Feb 23, 2015 at 03:55:56PM +0000, Tvrtko Ursulin wrote:
>> 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ä)
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
>>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
>>   drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
>>   3 files changed, 95 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 6e70748..a523d84 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2728,6 +2728,34 @@ 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 (gen2)
> 	return 128;

Okay I'll add it blindly, but again, I failed to find this in existing code.

>> +		return 512;
>> +	case I915_FORMAT_MOD_Y_TILED:
>> +		return 128;
>
> In theory we could check gen2 and HAS_128_BYTE_Y_TILING() here, but
> since old platforms didn't do Y tiled scanout anyway that's not really
> needed. But maybe toss in a comment about that so that people don't
> start wondering why this doesn't care about that stuff?

Will do.

Regards,

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

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

[Tvrtko Ursulin]

On 02/24/2015 05:26 PM, Damien Lespiau wrote:
> On Mon, Feb 23, 2015 at 03:55:59PM +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)
>>
>> 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>
>> ---
>
> This patch looks like it could reuse the newly introduced
> intel_fb_stride_alignment(). Otherwise:
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Thanks, I'll see how it looks with intel_fb_stride_alignment and 
potentially respin.

Regards,

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

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

[Tvrtko Ursulin]

On 02/24/2015 09:42 PM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 03:56:00PM +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.
>>
>> 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      | 33 ++++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
>>   4 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c622b11..74d4923 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11985,6 +11985,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;
>
> Imo this is fragile. We should unconditionally recomupte watermarks imo
> and just ellide the hw update if nothing changed. Spending a few cpu
> cycles on this per frame shouldn't hurt, and it will avoid duplicated
> logic and checks for watermark recomputation splattered all over the code.
 >
> I know that it's kinda there already, but still minimal enough to patch
> up. But if Matt knows that this will get removed again with the two-stage
> wm code then I'm ok with leaving it in, with a FIXME comment added.

"Splattered all over the code" equals two places going to one when 
primary and sprite planes are unified.

I wouldn't wish for this to become an unbounded task since I am not sure 
it is not good enough as it is. I checked with Matt and Ander and they 
think it is good enough for now.

Not sure if FIXME is warranted here - to say what? That it will be 
refactored with the two-stage wm rewrite?

Regards,

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

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

[Tvrtko Ursulin]

On 02/24/2015 09:46 PM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 03:56:01PM +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.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 74d4923..f100086 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12781,6 +12781,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;
>> @@ -12790,11 +12798,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;
>> -	}
>
> Imo the clearer code would be to add a
> 	
> 	switch (mode_cmd->modifier[0]) {
>
> 	}
>
> here and then shovel the platform checks into the supgroups. At least
> that's how we tend to roll these since it reduces the risks that a case is
> forgotten when the enumeration is extended. That would also have caught
> that we don't reject random garbage in the default: case.

Yes that's true - I'll see what I can do.

Regards,

Tvrtko

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

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

[Tvrtko Ursulin]

On 02/24/2015 04:54 PM, Damien Lespiau wrote:
> On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
>> 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)
>>
>> 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>
>
> Might want to add a MISSING_CASE() here as well. For the record, the

Will do.

> vertical alignments for Yf are taken from a dodgy looking document found
> by chance.
>
> We don't have a 128bpp format supported by display

Ok but we can't really error out from this helper so what do you suggest?

> Now that I think about it, for the horizontal stride, BSpec says:
>
>    - 8 bpp -> 64 bytes
>    - * bpp -> 128 bytes
>
> Given that a tile is a page size this would give for the vertical
> alignment:
>
>    - 8 bpp -> 64 byes (that's what we have)
>    - * -> 32 bytes
>
> So the 64bpp cases look a bit suspicious. Given that one the goals for
> this new tiling format is to have tiles as square as possible (in terms
> of pixels, not byte size), it'd make sense to have different strides
> constraints for the 64bpp format, so it could be BSpec being wrong on
> the horizontal stride constraint for 64bpp?

It's either square or 2:1 rectangular. I'll try to double check the 
numbers for 64bpp then.

Regards,

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

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

[Ville Syrjälä]

On Wed, Feb 25, 2015 at 10:36:38AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/24/2015 05:36 PM, Ville Syrjälä wrote:
> > On Mon, Feb 23, 2015 at 03:55:56PM +0000, Tvrtko Ursulin wrote:
> >> 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ä)
> >>
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
> >>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
> >>   drivers/gpu/drm/i915/intel_sprite.c  |  18 ++++--
> >>   3 files changed, 95 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 6e70748..a523d84 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -2728,6 +2728,34 @@ 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 (gen2)
> > 	return 128;
> 
> Okay I'll add it blindly, but again, I failed to find this in existing code.

i915_tiling_ok()

> 
> >> +		return 512;
> >> +	case I915_FORMAT_MOD_Y_TILED:
> >> +		return 128;
> >
> > In theory we could check gen2 and HAS_128_BYTE_Y_TILING() here, but
> > since old platforms didn't do Y tiled scanout anyway that's not really
> > needed. But maybe toss in a comment about that so that people don't
> > start wondering why this doesn't care about that stuff?
> 
> Will do.
> 
> Regards,
> 
> Tvrtko

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Damien Lespiau]

On Wed, Feb 25, 2015 at 10:54:31AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/24/2015 04:54 PM, Damien Lespiau wrote:
> >On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
> >>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)
> >>
> >>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>
> >
> >Might want to add a MISSING_CASE() here as well. For the record, the
> 
> Will do.
> 
> >vertical alignments for Yf are taken from a dodgy looking document found
> >by chance.
> >
> >We don't have a 128bpp format supported by display
> 
> Ok but we can't really error out from this helper so what do you suggest?

We could just remove the 128 case, we can't reach it.

> >Now that I think about it, for the horizontal stride, BSpec says:
> >
> >   - 8 bpp -> 64 bytes
> >   - * bpp -> 128 bytes
> >
> >Given that a tile is a page size this would give for the vertical
> >alignment:
> >
> >   - 8 bpp -> 64 byes (that's what we have)
> >   - * -> 32 bytes
> >
> >So the 64bpp cases look a bit suspicious. Given that one the goals for
> >this new tiling format is to have tiles as square as possible (in terms
> >of pixels, not byte size), it'd make sense to have different strides
> >constraints for the 64bpp format, so it could be BSpec being wrong on
> >the horizontal stride constraint for 64bpp?
> 
> It's either square or 2:1 rectangular. I'll try to double check the numbers
> for 64bpp then.

Thanks. Thinking about it a bit more, it could just be that the display
engine has a slightly stricter constraint than the 3D pipeline for the
alignment of 64bpc fbs. And so the code would be fine. It's all
assumptions though.

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

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

[Damien Lespiau]

On Wed, Feb 25, 2015 at 10:34:19AM +0000, Tvrtko Ursulin wrote:
> >While this was what was documented at some point, it's not anymore.
> >Would need an extra patch before this one to introduce
> >plane_blocks_per_line and then this one on top.
> 
> Don't follow - what is plane_blocks_per_line and where and how would you add
> it before this patch?

It's all in the specs (the WM documentation has been updated last week).
plane_blocks_per_line has been introduced in the update. X and linear
tiling uses plane_blocks_per_line as well, so a patch that introduces it
before this patch would make sense.

FWIW, the spec update is to deal with Y tile underflow in some cases,
which sounds like something we want.

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

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

[Damien Lespiau]

On Wed, Feb 25, 2015 at 10:50:04AM +0000, Tvrtko Ursulin wrote:
> Not sure if FIXME is warranted here - to say what? That it will be
> refactored with the two-stage wm rewrite?

Something like that. How about?

"FIXME: The plan for WM updates is to be always computed at check time
to make sure we can actually update the WMs for this configuration
before the commit (For instance we could fail to allocate the mininum
space for a plane in the DDB). But until then we just signal that WMs
have to be updated somewhere down the line (in the commit phase)."

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

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

[Daniel Vetter]

On Wed, Feb 25, 2015 at 10:50:04AM +0000, Tvrtko Ursulin wrote:
> On 02/24/2015 09:42 PM, Daniel Vetter wrote:
> >On Mon, Feb 23, 2015 at 03:56:00PM +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.
> >>
> >>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      | 33 ++++++++++++++++++++++++++++-----
> >>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
> >>  4 files changed, 41 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index c622b11..74d4923 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -11985,6 +11985,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;
> >
> >Imo this is fragile. We should unconditionally recomupte watermarks imo
> >and just ellide the hw update if nothing changed. Spending a few cpu
> >cycles on this per frame shouldn't hurt, and it will avoid duplicated
> >logic and checks for watermark recomputation splattered all over the code.
> >
> >I know that it's kinda there already, but still minimal enough to patch
> >up. But if Matt knows that this will get removed again with the two-stage
> >wm code then I'm ok with leaving it in, with a FIXME comment added.
> 
> "Splattered all over the code" equals two places going to one when primary
> and sprite planes are unified.

Yeah splattered was over the top, it's really just the duplication between
the implicit "wm state changed because values are different" and what we
think all affects wm state. Someone somewhere will forgot to double-check
this and then suddenly some pageflip with different fb formats fails with
underruns. But since everyone just tests with full modeset that never gets
noticed in testing.

> I wouldn't wish for this to become an unbounded task since I am not sure it
> is not good enough as it is. I checked with Matt and Ander and they think it
> is good enough for now.
> 
> Not sure if FIXME is warranted here - to say what? That it will be
> refactored with the two-stage wm rewrite?

I guess I could smash a fixme comment patch on top so I don't forget. If
Ander&Matt are ok with this until full two-stage wm update has landed I'm
ok too.

Thanks, 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 3/7] drm/i915/skl: Adjust intel_fb_align_height() for Yb/Yf tiling

[Damien Lespiau]

On Wed, Feb 25, 2015 at 02:00:18PM +0000, Damien Lespiau wrote:
> > It's either square or 2:1 rectangular. I'll try to double check the numbers
> > for 64bpp then.
> 
> Thanks. Thinking about it a bit more, it could just be that the display
> engine has a slightly stricter constraint than the 3D pipeline for the
> alignment of 64bpc fbs. And so the code would be fine. It's all
> assumptions though.

Given that it could be just that and we're doing things as they are
documented, you can add my:

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