[PATCH v4 0/7] Skylake 90/270 display rotation

[PATCH v4 0/7] Skylake 90/270 display rotation

[Tvrtko Ursulin]

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

Display engine on Skylake can scan out specially prepared frame buffers
rotated by 90 or 270 degrees.

This adds partial support for this and will need some patches from Sonika to
complete the feature.

Going by "looking pretty" comment by Daniel Vetter (on IRC) I think this is now
ready for detailed review.

v2:
   * Individual review comments.
   * Main patch split into four smaller ones.

v3:
   * Dropped the DRM core patch since it has been merged.
   * Refactored tile height helper a bit.
   * Moved rotated GGTT view code into i915_gem_gtt.c

v4:
   * Rebased for ggtt view API changes.

Tvrtko Ursulin (7):
  drm/i915/skl: Extract tile height code into a helper function
  drm/i915: Use GGTT view when (un)pinning objects to planes
  drm/i915: Pass in plane state when (un)pinning frame buffers
  drm/i915: Helper function to determine GGTT view from plane state
  drm/i915/skl: Support secondary (rotated) frame buffer mapping
  drm/i915/skl: Query display address through a wrapper
  drm/i915/skl: Take 90/270 rotation into account in watermark
    calculations

 drivers/gpu/drm/i915/i915_drv.h      |  14 ++-
 drivers/gpu/drm/i915/i915_gem.c      |  22 +++--
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 113 ++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_gtt.h  |  12 +++
 drivers/gpu/drm/i915/intel_display.c | 159 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  25 +++++-
 drivers/gpu/drm/i915/intel_fbdev.c   |   2 +-
 drivers/gpu/drm/i915/intel_overlay.c |   3 +-
 drivers/gpu/drm/i915/intel_pm.c      |  18 +++-
 drivers/gpu/drm/i915/intel_sprite.c  |  10 +--
 10 files changed, 319 insertions(+), 59 deletions(-)

-- 
2.3.2

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

[PATCH 1/7] drm/i915/skl: Extract tile height code into a helper function

[Tvrtko Ursulin]

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

It will be used in a later patch and also convert all height parameters
from int to unsigned int.

v2: Rebased for fb modifiers.
v3: Fixed v2 rebase.
v4:
   * Height should be unsigned int.
   * Make it take pixel_format for consistency and simplicity.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v1)
---
 drivers/gpu/drm/i915/intel_display.c | 43 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  7 +++---
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 90b460c..a307979 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2194,13 +2194,12 @@ static bool need_vtd_wa(struct drm_device *dev)
 	return false;
 }
 
-int
-intel_fb_align_height(struct drm_device *dev, int height,
-		      uint32_t pixel_format,
-		      uint64_t fb_format_modifier)
+static unsigned int
+intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
+		  uint64_t fb_format_modifier)
 {
-	int tile_height;
-	uint32_t bits_per_pixel;
+	unsigned int tile_height;
+	uint32_t pixel_bytes;
 
 	switch (fb_format_modifier) {
 	case DRM_FORMAT_MOD_NONE:
@@ -2213,20 +2212,20 @@ intel_fb_align_height(struct drm_device *dev, int height,
 		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) {
+		pixel_bytes = drm_format_plane_cpp(pixel_format, 0);
+		switch (pixel_bytes) {
 		default:
-		case 8:
+		case 1:
 			tile_height = 64;
 			break;
-		case 16:
-		case 32:
+		case 2:
+		case 4:
 			tile_height = 32;
 			break;
-		case 64:
+		case 8:
 			tile_height = 16;
 			break;
-		case 128:
+		case 16:
 			WARN_ONCE(1,
 				  "128-bit pixels are not supported for display!");
 			tile_height = 16;
@@ -2239,7 +2238,15 @@ intel_fb_align_height(struct drm_device *dev, int height,
 		break;
 	}
 
-	return ALIGN(height, tile_height);
+	return tile_height;
+}
+
+unsigned int
+intel_fb_align_height(struct drm_device *dev, unsigned int height,
+		      uint32_t pixel_format, uint64_t fb_format_modifier)
+{
+	return ALIGN(height, intel_tile_height(dev, pixel_format,
+					       fb_format_modifier));
 }
 
 int
@@ -6772,7 +6779,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 	u32 val, base, offset;
 	int pipe = crtc->pipe, plane = crtc->plane;
 	int fourcc, pixel_format;
-	int aligned_height;
+	unsigned int aligned_height;
 	struct drm_framebuffer *fb;
 	struct intel_framebuffer *intel_fb;
 
@@ -7810,7 +7817,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	u32 val, base, offset, stride_mult, tiling;
 	int pipe = crtc->pipe;
 	int fourcc, pixel_format;
-	int aligned_height;
+	unsigned int aligned_height;
 	struct drm_framebuffer *fb;
 	struct intel_framebuffer *intel_fb;
 
@@ -7918,7 +7925,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 	u32 val, base, offset;
 	int pipe = crtc->pipe;
 	int fourcc, pixel_format;
-	int aligned_height;
+	unsigned int aligned_height;
 	struct drm_framebuffer *fb;
 	struct intel_framebuffer *intel_fb;
 
@@ -12849,7 +12856,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
 				  struct drm_i915_gem_object *obj)
 {
-	int aligned_height;
+	unsigned int aligned_height;
 	int ret;
 	u32 pitch_limit, stride_alignment;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a1baaa1..5254540 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -904,9 +904,10 @@ void intel_frontbuffer_flip(struct drm_device *dev,
 	intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
 
-int intel_fb_align_height(struct drm_device *dev, int height,
-			  uint32_t pixel_format,
-			  uint64_t fb_format_modifier);
+unsigned int intel_fb_align_height(struct drm_device *dev,
+				   unsigned int height,
+				   uint32_t pixel_format,
+				   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,
-- 
2.3.2

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

[PATCH 2/7] drm/i915: Use GGTT view when (un)pinning objects to planes

[Tvrtko Ursulin]

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

To support frame buffer rotation we need to be able to pass on the information
on what kind of GGTT view is required for display.

This patch just adds the parameter and makes all the callers default to the
normal view.

v2: Rebased for ggtt view changes.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 14 +++++++++++---
 drivers/gpu/drm/i915/i915_gem.c      | 22 ++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c |  7 ++++---
 drivers/gpu/drm/i915/intel_overlay.c |  3 ++-
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 81f60b4..19b9e69 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2772,8 +2772,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
 int __must_check
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     u32 alignment,
-				     struct intel_engine_cs *pipelined);
-void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
+				     struct intel_engine_cs *pipelined,
+				     const struct i915_ggtt_view *view);
+void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
+					      const struct i915_ggtt_view *view);
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
 				int align);
 int i915_gem_open(struct drm_device *dev, struct drm_file *file);
@@ -2882,7 +2884,13 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
 	return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
 }
 
-void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
+void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
+				     const struct i915_ggtt_view *view);
+static inline void
+i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
+{
+	i915_gem_object_ggtt_unpin_view(obj, &i915_ggtt_view_normal);
+}
 
 /* i915_gem_context.c */
 int __must_check i915_gem_context_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 533ef37..58723a3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3957,7 +3957,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
 int
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     u32 alignment,
-				     struct intel_engine_cs *pipelined)
+				     struct intel_engine_cs *pipelined,
+				     const struct i915_ggtt_view *view)
 {
 	u32 old_read_domains, old_write_domain;
 	bool was_pin_display;
@@ -3993,7 +3994,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	 * (e.g. libkms for the bootup splash), we have to ensure that we
 	 * always use map_and_fenceable for all scanout buffers.
 	 */
-	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
+	ret = i915_gem_object_ggtt_pin(obj, view, alignment,
+				       view->type == I915_GGTT_VIEW_NORMAL ?
+				       PIN_MAPPABLE : 0);
 	if (ret)
 		goto err_unpin_display;
 
@@ -4021,9 +4024,11 @@ err_unpin_display:
 }
 
 void
-i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
+i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
+					 const struct i915_ggtt_view *view)
 {
-	i915_gem_object_ggtt_unpin(obj);
+	i915_gem_object_ggtt_unpin_view(obj, view);
+
 	obj->pin_display = is_pin_display(obj);
 }
 
@@ -4296,15 +4301,16 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
 }
 
 void
-i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
+i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
+				const struct i915_ggtt_view *view)
 {
-	struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
+	struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
 
 	BUG_ON(!vma);
 	BUG_ON(vma->pin_count == 0);
-	BUG_ON(!i915_gem_obj_ggtt_bound(obj));
+	BUG_ON(!i915_gem_obj_ggtt_bound_view(obj, view->type));
 
-	if (--vma->pin_count == 0)
+	if (--vma->pin_count == 0 && view->type == I915_GGTT_VIEW_NORMAL)
 		obj->pin_mappable = false;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a307979..16f3443 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2311,7 +2311,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	intel_runtime_pm_get(dev_priv);
 
 	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
+						   &i915_ggtt_view_normal);
 	if (ret)
 		goto err_interruptible;
 
@@ -2331,7 +2332,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	return 0;
 
 err_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
+	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
@@ -2343,7 +2344,7 @@ static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
 
 	i915_gem_object_unpin_fence(obj);
-	i915_gem_object_unpin_from_display_plane(obj);
+	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 823d1d9..dd92122 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL,
+						   &i915_ggtt_view_normal);
 	if (ret != 0)
 		return ret;
 
-- 
2.3.2

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

[PATCH 3/7] drm/i915: Pass in plane state when (un)pinning frame buffers

[Tvrtko Ursulin]

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

Plane state carries the rotation information which is needed for determining
the appropriate GGTT view type.

This just adds the parameter with the actual usage coming in future patches.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16f3443..862aa46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2252,6 +2252,7 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
 int
 intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			   struct drm_framebuffer *fb,
+			   const struct drm_plane_state *plane_state,
 			   struct intel_engine_cs *pipelined)
 {
 	struct drm_device *dev = fb->dev;
@@ -2339,8 +2340,11 @@ err_interruptible:
 	return ret;
 }
 
-static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
+static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
+			       const struct drm_plane_state *plane_state)
 {
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+
 	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
 
 	i915_gem_object_unpin_fence(obj);
@@ -9277,7 +9281,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
+	intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state);
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 
 	intel_fbc_update(dev);
@@ -9985,7 +9989,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		ring = &dev_priv->ring[RCS];
 	}
 
-	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring);
+	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
+					 crtc->primary->state, ring);
 	if (ret)
 		goto cleanup_pending;
 
@@ -10025,7 +10030,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	return 0;
 
 cleanup_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fb_obj(fb, crtc->primary->state);
 cleanup_pending:
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
@@ -11981,7 +11986,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		if (ret)
 			DRM_DEBUG_KMS("failed to attach phys object\n");
 	} else {
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL);
 	}
 
 	if (ret == 0)
@@ -12013,7 +12018,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical) {
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(obj);
+		intel_unpin_fb_obj(fb, old_state);
 		mutex_unlock(&dev->struct_mutex);
 	}
 }
@@ -13906,6 +13911,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 
 		if (intel_pin_and_fence_fb_obj(c->primary,
 					       c->primary->fb,
+					       c->primary->state,
 					       NULL)) {
 			DRM_ERROR("failed to pin boot fb on pipe %d\n",
 				  to_intel_crtc(c)->pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5254540..3721878 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -962,6 +962,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old);
 int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			       struct drm_framebuffer *fb,
+			       const struct drm_plane_state *plane_state,
 			       struct intel_engine_cs *pipelined);
 struct drm_framebuffer *
 __intel_framebuffer_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 757c0d2..4e7e7da 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -151,7 +151,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	}
 
 	/* Flush everything out, we'll be doing GTT only from now on */
-	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
+	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
 		goto out_fb;
-- 
2.3.2

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

[PATCH 4/7] drm/i915: Helper function to determine GGTT view from plane state

[Tvrtko Ursulin]

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

For now only default implementation defaulting to normal view.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 862aa46..fe11e99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2249,6 +2249,16 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
 					       fb_format_modifier));
 }
 
+static
+int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
+			    struct drm_framebuffer *fb,
+			    const struct drm_plane_state *plane_state)
+{
+	*view = i915_ggtt_view_normal;
+
+	return 0;
+}
+
 int
 intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			   struct drm_framebuffer *fb,
@@ -2258,6 +2268,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	struct drm_device *dev = fb->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct i915_ggtt_view view;
 	u32 alignment;
 	int ret;
 
@@ -2294,6 +2305,10 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
+	if (ret < 0)
+		return ret;
+
 	/* Note that the w/a also requires 64 PTE of padding following the
 	 * bo. We currently fill all unused PTE with the shadow page and so
 	 * we should always have valid PTE following the scanout preventing
@@ -2313,7 +2328,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
-						   &i915_ggtt_view_normal);
+						   &view);
 	if (ret)
 		goto err_interruptible;
 
@@ -2333,7 +2348,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	return 0;
 
 err_unpin:
-	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
+	i915_gem_object_unpin_from_display_plane(obj, &view);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
@@ -2344,11 +2359,16 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
 			       const struct drm_plane_state *plane_state)
 {
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct i915_ggtt_view view;
+	int ret;
 
 	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
 
+	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
+	WARN_ONCE(ret < 0, "Couldn't get view from plane state!");
+
 	i915_gem_object_unpin_fence(obj);
-	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
+	i915_gem_object_unpin_from_display_plane(obj, &view);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
-- 
2.3.2

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

[PATCH 5/7] drm/i915/skl: Support secondary (rotated) frame buffer mapping

[Tvrtko Ursulin]

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

90/270 rotated scanout needs a rotated GTT view of the framebuffer.

This is put in a separate VMA with a dedicated ggtt view and wired such that
it is created when a framebuffer is pinned to a 90/270 rotated plane.

Rotation is only possible with Yb/Yf buffers and error is propagated to
user space in case of a mismatch.

Special rotated page view is constructed at the VMA creation time by
borrowing the DMA addresses from obj->pages.

v2:
    * Do not bother with pages for rotated sg list, just populate the DMA
      addresses. (Daniel Vetter)
    * Checkpatch cleanup.

v3:
    * Rebased on top of new plane handling (create rotated mapping when
      setting the rotation property).
    * Unpin rotated VMA on unpinning from display plane.
    * Simplify rotation check using bitwise AND. (Chris Wilson)

v4:
    * Fix unpinning of optional rotated mapping so it is really considered
      to be optional.

v5:
   * Rebased for fb modifier changes.
   * Rebased for atomic commit.
   * Only pin needed view for display. (Ville Syrjälä, Daniel Vetter)

v6:
   * Rebased after preparatory work has been extracted out. (Daniel Vetter)

v7:
   * Slightly simplified tiling geometry calculation.
   * Moved rotated GGTT view implementation into i915_gem_gtt.c (Daniel Vetter)

v8:
   * Do not use i915_gem_obj_size to get object size since that actually
     returns the size of an VMA which may not exist.
   * Rebased for ggtt view changes.

For: VIZ-4726
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 113 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_gtt.h  |  12 ++++
 drivers/gpu/drm/i915/intel_display.c |  29 ++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   4 ++
 4 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f1b9ea6..5381ebf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2413,15 +2413,119 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
 
 }
 
+static
+void rotate_pages(dma_addr_t *in, unsigned int width, unsigned int height,
+		  struct sg_table *st)
+{
+	unsigned int column, row;
+	unsigned int src_idx;
+	struct scatterlist *sg = st->sgl;
+
+	st->nents = 0;
+
+	for (column = 0; column < width; column++) {
+		src_idx = width * (height - 1) + column;
+		for (row = 0; row < height; row++) {
+			st->nents++;
+			/* We don't need the pages, but need to initialize
+			 * the entries so the sg list can be happily traversed.
+			 * The only thing we need are DMA addresses.
+			 */
+			sg_set_page(sg, NULL, PAGE_SIZE, 0);
+			sg_dma_address(sg) = in[src_idx];
+			sg_dma_len(sg) = PAGE_SIZE;
+			sg = sg_next(sg);
+			src_idx -= width;
+		}
+	}
+}
+
+static
+struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
+					   struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
+	unsigned long size, pages, rot_pages;
+	struct sg_page_iter sg_iter;
+	unsigned long i;
+	dma_addr_t *page_addr_list;
+	struct sg_table *st;
+	unsigned int tile_pitch, tile_height;
+	unsigned int width_pages, height_pages;
+	int ret = ENOMEM;
+
+	pages = obj->base.size / PAGE_SIZE;
+
+	/* Calculate tiling geometry. */
+	tile_height = intel_tile_height(dev, rot_info->pixel_format,
+					rot_info->fb_modifier);
+	tile_pitch = PAGE_SIZE / tile_height;
+	width_pages = DIV_ROUND_UP(rot_info->pitch, tile_pitch);
+	height_pages = DIV_ROUND_UP(rot_info->height, tile_height);
+	rot_pages = width_pages * height_pages;
+	size = rot_pages * PAGE_SIZE;
+
+	/* Allocate a temporary list of source pages for random access. */
+	page_addr_list = drm_malloc_ab(pages, sizeof(dma_addr_t));
+	if (!page_addr_list)
+		return ERR_PTR(ret);
+
+	/* Allocate target SG list. */
+	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	if (!st)
+		goto err_st_alloc;
+
+	ret = sg_alloc_table(st, rot_pages, GFP_KERNEL);
+	if (ret)
+		goto err_sg_alloc;
+
+	/* Populate source page list from the object. */
+	i = 0;
+	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
+		page_addr_list[i] = sg_page_iter_dma_address(&sg_iter);
+		i++;
+	}
+
+	/* Rotate the pages. */
+	rotate_pages(page_addr_list, width_pages, height_pages, st);
+
+	DRM_DEBUG_KMS(
+		      "Created rotated page mapping for object size %lu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages).\n",
+		      size, rot_info->pitch, rot_info->height,
+		      rot_info->pixel_format, width_pages, height_pages,
+		      rot_pages);
+
+	drm_free_large(page_addr_list);
+
+	return st;
+
+err_sg_alloc:
+	kfree(st);
+err_st_alloc:
+	drm_free_large(page_addr_list);
+
+	DRM_DEBUG_KMS(
+		      "Failed to create rotated mapping for object size %lu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages)\n",
+		      size, ret, rot_info->pitch, rot_info->height,
+		      rot_info->pixel_format, width_pages, height_pages,
+		      rot_pages);
+	return ERR_PTR(ret);
+}
 
 static inline
 int i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
+	int ret = 0;
+
 	if (vma->ggtt_view.pages)
 		return 0;
 
 	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
 		vma->ggtt_view.pages = vma->obj->pages;
+	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
+		vma->ggtt_view.pages =
+			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
 	else
 		WARN_ONCE(1, "GGTT view %u not implemented!\n",
 			  vma->ggtt_view.type);
@@ -2429,10 +2533,15 @@ int i915_get_ggtt_vma_pages(struct i915_vma *vma)
 	if (!vma->ggtt_view.pages) {
 		DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
 			  vma->ggtt_view.type);
-		return -EINVAL;
+		ret = -EINVAL;
+	} else if (IS_ERR(vma->ggtt_view.pages)) {
+		ret = PTR_ERR(vma->ggtt_view.pages);
+		vma->ggtt_view.pages = NULL;
+		DRM_ERROR("Failed to get pages for VMA view type %u (%d)!\n",
+			  vma->ggtt_view.type, ret);
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index c9e93f5..a61e8dd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -111,12 +111,24 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
 
 enum i915_ggtt_view_type {
 	I915_GGTT_VIEW_NORMAL = 0,
+	I915_GGTT_VIEW_ROTATED
+};
+
+struct intel_rotation_info {
+	unsigned int height;
+	unsigned int pitch;
+	uint32_t pixel_format;
+	uint64_t fb_modifier;
 };
 
 struct i915_ggtt_view {
 	enum i915_ggtt_view_type type;
 
 	struct sg_table *pages;
+
+	union {
+		struct intel_rotation_info rotation_info;
+	};
 };
 
 extern const struct i915_ggtt_view i915_ggtt_view_normal;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe11e99..c2c3a76 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
 	return false;
 }
 
-static unsigned int
+unsigned int
 intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
 		  uint64_t fb_format_modifier)
 {
@@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
 			    struct drm_framebuffer *fb,
 			    const struct drm_plane_state *plane_state)
 {
+	struct intel_rotation_info *info = &view->rotation_info;
+	static const struct i915_ggtt_view rotated_view =
+				{ .type = I915_GGTT_VIEW_ROTATED };
+
 	*view = i915_ggtt_view_normal;
 
-	return 0;
+	if (!plane_state)
+		return 0;
+
+	if (!(plane_state->rotation &
+	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
+		return 0;
+
+	*view = rotated_view;
+
+	info->height = fb->height;
+	info->pixel_format = fb->pixel_format;
+	info->pitch = fb->pitches[0];
+	info->fb_modifier = fb->modifier[0];
+
+	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
+	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
+		DRM_DEBUG_KMS(
+			      "Y or Yf tiling is needed for 90/270 rotation!\n");
+		return -EINVAL;
+	}
+
+	return 1;
 }
 
 int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3721878..53a1372 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -987,6 +987,10 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
 				    struct drm_property *property,
 				    uint64_t val);
 
+unsigned int
+intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
+		  uint64_t fb_format_modifier);
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
-- 
2.3.2

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

[PATCH 6/7] drm/i915/skl: Query display address through a wrapper

[Tvrtko Ursulin]

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

Need to do this in order to support 90/270 rotated display.

v2: Pass in drm_plane instead of plane index to intel_obj_display_address.

v3:
    * Renamed intel_obj_display_address to intel_plane_obj_offset.
      (Chris Wilson)
    * Simplified rotation check to bitwise AND. (Chris Wilson)

v4:
    * Extracted 90/270 rotation check into a helper function. (Michel Thierry)

v5:
    * Rebased for ggtt view changes.

For: VIZ-4545
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  9 +++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  5 ++++-
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c2c3a76..9af73b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2263,8 +2263,7 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
 	if (!plane_state)
 		return 0;
 
-	if (!(plane_state->rotation &
-	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
+	if (!intel_rotation_90_or_270(plane_state->rotation))
 		return 0;
 
 	*view = rotated_view;
@@ -2862,6 +2861,17 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
 	}
 }
 
+unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
+				     struct drm_i915_gem_object *obj)
+{
+	enum i915_ggtt_view_type view = I915_GGTT_VIEW_NORMAL;
+
+	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
+		view = I915_GGTT_VIEW_ROTATED;
+
+	return i915_gem_obj_ggtt_offset_view(obj, view);
+}
+
 static void skylake_update_primary_plane(struct drm_crtc *crtc,
 					 struct drm_framebuffer *fb,
 					 int x, int y)
@@ -2872,6 +2882,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj;
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl, stride_div;
+	unsigned long surf_addr;
 
 	if (!intel_crtc->primary_enabled) {
 		I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -2938,16 +2949,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	obj = intel_fb_obj(fb);
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
+	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
-
 	I915_WRITE(PLANE_POS(pipe, 0), 0);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
 	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), fb->pitches[0] / stride_div);
-	I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
+	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
 
 	POSTING_READ(PLANE_SURF(pipe, 0));
 }
@@ -10039,8 +10050,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_pending;
 
-	work->gtt_offset =
-		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
+	work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary), obj)
+						  + intel_crtc->dspaddr_offset;
 
 	if (use_mmio_flip(ring, obj)) {
 		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 53a1372..ae28fb4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -991,6 +991,12 @@ unsigned int
 intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
 		  uint64_t fb_format_modifier);
 
+static inline bool
+intel_rotation_90_or_270(unsigned int rotation)
+{
+	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
+}
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
@@ -1045,6 +1051,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 
+unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
+				     struct drm_i915_gem_object *obj);
+
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
 bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a828736..d960572 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -191,6 +191,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	unsigned long surf_addr;
 
 	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
 
@@ -280,12 +281,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	crtc_w--;
 	crtc_h--;
 
+	surf_addr = intel_plane_obj_offset(intel_plane, obj);
+
 	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
 	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);
-	I915_WRITE(PLANE_SURF(pipe, plane), i915_gem_obj_ggtt_offset(obj));
+	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
 	POSTING_READ(PLANE_SURF(pipe, plane));
 }
 
-- 
2.3.2

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

[PATCH 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[Tvrtko Ursulin]

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

v2: Pass in rotation info to sprite plane updates as well.

v3: Use helper to determine 90/270 rotation. (Michel Thierry)

v4: Rebased for fb modifiers and atomic changes.

For: VIZ-4546
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 18 +++++++++++++++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
 4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9af73b1..19bec33 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11994,6 +11994,28 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 }
 
 /**
+ * intel_wm_need_update - Check whether watermarks need updating
+ * @plane: drm plane
+ * @state: new plane state
+ *
+ * Check current plane state versus the new one to determine whether
+ * watermarks need to be recalculated.
+ *
+ * Returns true or false.
+ */
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	/* Update watermarks on tiling changes. */
+	if (!plane->state->fb || !state->fb ||
+	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
+	    plane->state->rotation != state->rotation)
+		return true;
+
+	return false;
+}
+
+/**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
  * @fb: framebuffer to prepare for presentation
@@ -12139,10 +12161,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 		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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ae28fb4..a7ecf95 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 {
 	bool enabled;
 	bool scaled;
 	u64 tiling;
+	unsigned int rotation;
 };
 
 struct intel_plane {
@@ -997,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
 	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
 }
 
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state);
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 288c9d2..4e080a6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2840,6 +2840,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		}
 		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].rotation = crtc->primary->state->rotation;
 
 		fb = crtc->cursor->state->fb;
 		if (fb) {
@@ -2897,7 +2898,21 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
 	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t y_tile_minimum = plane_blocks_per_line * 4;
+		uint32_t min_scanlines = 4;
+		uint32_t y_tile_minimum;
+		if (intel_rotation_90_or_270(p_params->rotation)) {
+			switch (p_params->bytes_per_pixel) {
+			case 1:
+				min_scanlines = 16;
+				break;
+			case 2:
+				min_scanlines = 8;
+				break;
+			case 8:
+				WARN(1, "Unsupported pixel depth for rotation");
+			};
+		}
+		y_tile_minimum = plane_blocks_per_line * min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3357,6 +3372,7 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	if (fb)
 		intel_plane->wm.tiling = fb->modifier[0];
+	intel_plane->wm.rotation = plane->state->rotation;
 
 	skl_update_wm(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d960572..b919b0a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1260,10 +1260,7 @@ 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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 	}
 
-- 
2.3.2

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

Re: [PATCH 1/7] drm/i915/skl: Extract tile height code into a helper function

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It will be used in a later patch and also convert all height parameters
> from int to unsigned int.
> 
> v2: Rebased for fb modifiers.
> v3: Fixed v2 rebase.
> v4:
>    * Height should be unsigned int.
>    * Make it take pixel_format for consistency and simplicity.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v1)
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v4)
> ---
>  drivers/gpu/drm/i915/intel_display.c | 43 +++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 +++---
>  2 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 90b460c..a307979 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2194,13 +2194,12 @@ static bool need_vtd_wa(struct drm_device *dev)
>  	return false;
>  }
>  
> -int
> -intel_fb_align_height(struct drm_device *dev, int height,
> -		      uint32_t pixel_format,
> -		      uint64_t fb_format_modifier)
> +static unsigned int
> +intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> +		  uint64_t fb_format_modifier)
>  {
> -	int tile_height;
> -	uint32_t bits_per_pixel;
> +	unsigned int tile_height;
> +	uint32_t pixel_bytes;
>  
>  	switch (fb_format_modifier) {
>  	case DRM_FORMAT_MOD_NONE:
> @@ -2213,20 +2212,20 @@ intel_fb_align_height(struct drm_device *dev, int height,
>  		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) {
> +		pixel_bytes = drm_format_plane_cpp(pixel_format, 0);
> +		switch (pixel_bytes) {
>  		default:
> -		case 8:
> +		case 1:
>  			tile_height = 64;
>  			break;
> -		case 16:
> -		case 32:
> +		case 2:
> +		case 4:
>  			tile_height = 32;
>  			break;
> -		case 64:
> +		case 8:
>  			tile_height = 16;
>  			break;
> -		case 128:
> +		case 16:
>  			WARN_ONCE(1,
>  				  "128-bit pixels are not supported for display!");
>  			tile_height = 16;
> @@ -2239,7 +2238,15 @@ intel_fb_align_height(struct drm_device *dev, int height,
>  		break;
>  	}
>  
> -	return ALIGN(height, tile_height);
> +	return tile_height;
> +}
> +
> +unsigned int
> +intel_fb_align_height(struct drm_device *dev, unsigned int height,
> +		      uint32_t pixel_format, uint64_t fb_format_modifier)
> +{
> +	return ALIGN(height, intel_tile_height(dev, pixel_format,
> +					       fb_format_modifier));
>  }
>  
>  int
> @@ -6772,7 +6779,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  	u32 val, base, offset;
>  	int pipe = crtc->pipe, plane = crtc->plane;
>  	int fourcc, pixel_format;
> -	int aligned_height;
> +	unsigned int aligned_height;
>  	struct drm_framebuffer *fb;
>  	struct intel_framebuffer *intel_fb;
>  
> @@ -7810,7 +7817,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	u32 val, base, offset, stride_mult, tiling;
>  	int pipe = crtc->pipe;
>  	int fourcc, pixel_format;
> -	int aligned_height;
> +	unsigned int aligned_height;
>  	struct drm_framebuffer *fb;
>  	struct intel_framebuffer *intel_fb;
>  
> @@ -7918,7 +7925,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>  	u32 val, base, offset;
>  	int pipe = crtc->pipe;
>  	int fourcc, pixel_format;
> -	int aligned_height;
> +	unsigned int aligned_height;
>  	struct drm_framebuffer *fb;
>  	struct intel_framebuffer *intel_fb;
>  
> @@ -12849,7 +12856,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
>  				  struct drm_i915_gem_object *obj)
>  {
> -	int aligned_height;
> +	unsigned int aligned_height;
>  	int ret;
>  	u32 pitch_limit, stride_alignment;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a1baaa1..5254540 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -904,9 +904,10 @@ void intel_frontbuffer_flip(struct drm_device *dev,
>  	intel_frontbuffer_flush(dev, frontbuffer_bits);
>  }
>  
> -int intel_fb_align_height(struct drm_device *dev, int height,
> -			  uint32_t pixel_format,
> -			  uint64_t fb_format_modifier);
> +unsigned int intel_fb_align_height(struct drm_device *dev,
> +				   unsigned int height,
> +				   uint32_t pixel_format,
> +				   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,


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

Re: [PATCH 2/7] drm/i915: Use GGTT view when (un)pinning objects to planes

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> To support frame buffer rotation we need to be able to pass on the information
> on what kind of GGTT view is required for display.
> 
> This patch just adds the parameter and makes all the callers default to the
> normal view.
> 
> v2: Rebased for ggtt view changes.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 14 +++++++++++---
>  drivers/gpu/drm/i915/i915_gem.c      | 22 ++++++++++++++--------
>  drivers/gpu/drm/i915/intel_display.c |  7 ++++---
>  drivers/gpu/drm/i915/intel_overlay.c |  3 ++-
>  4 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 81f60b4..19b9e69 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2772,8 +2772,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>  int __must_check
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     u32 alignment,
> -				     struct intel_engine_cs *pipelined);
> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
> +				     struct intel_engine_cs *pipelined,
> +				     const struct i915_ggtt_view *view);
> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> +					      const struct i915_ggtt_view *view);
>  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>  				int align);
>  int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> @@ -2882,7 +2884,13 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>  	return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
>  }
>  
> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> +				     const struct i915_ggtt_view *view);
> +static inline void
> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> +{
> +	i915_gem_object_ggtt_unpin_view(obj, &i915_ggtt_view_normal);
> +}
>  
>  /* i915_gem_context.c */
>  int __must_check i915_gem_context_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 533ef37..58723a3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3957,7 +3957,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>  int
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     u32 alignment,
> -				     struct intel_engine_cs *pipelined)
> +				     struct intel_engine_cs *pipelined,
> +				     const struct i915_ggtt_view *view)
>  {
>  	u32 old_read_domains, old_write_domain;
>  	bool was_pin_display;
> @@ -3993,7 +3994,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 * (e.g. libkms for the bootup splash), we have to ensure that we
>  	 * always use map_and_fenceable for all scanout buffers.
>  	 */
> -	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> +	ret = i915_gem_object_ggtt_pin(obj, view, alignment,
> +				       view->type == I915_GGTT_VIEW_NORMAL ?
> +				       PIN_MAPPABLE : 0);

I'm slightly concerned about making an assumption that other but normal
views need not to be mappable (when none are defined). As discussed in
IRC, this should be moved later into the series when we actually know
about the other views.

>  	if (ret)
>  		goto err_unpin_display;
>  
> @@ -4021,9 +4024,11 @@ err_unpin_display:
>  }
>  
>  void
> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> +					 const struct i915_ggtt_view *view)
>  {
> -	i915_gem_object_ggtt_unpin(obj);
> +	i915_gem_object_ggtt_unpin_view(obj, view);
> +
>  	obj->pin_display = is_pin_display(obj);
>  }
>  
> @@ -4296,15 +4301,16 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>  }
>  
>  void
> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> +				const struct i915_ggtt_view *view)
>  {
> -	struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
> +	struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>  
>  	BUG_ON(!vma);
>  	BUG_ON(vma->pin_count == 0);
> -	BUG_ON(!i915_gem_obj_ggtt_bound(obj));
> +	BUG_ON(!i915_gem_obj_ggtt_bound_view(obj, view->type));
>  
> -	if (--vma->pin_count == 0)
> +	if (--vma->pin_count == 0 && view->type == I915_GGTT_VIEW_NORMAL)
>  		obj->pin_mappable = false;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a307979..16f3443 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2311,7 +2311,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	intel_runtime_pm_get(dev_priv);
>  
>  	dev_priv->mm.interruptible = false;
> -	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
> +	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
> +						   &i915_ggtt_view_normal);
>  	if (ret)
>  		goto err_interruptible;
>  
> @@ -2331,7 +2332,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	return 0;
>  
>  err_unpin:
> -	i915_gem_object_unpin_from_display_plane(obj);
> +	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
>  err_interruptible:
>  	dev_priv->mm.interruptible = true;
>  	intel_runtime_pm_put(dev_priv);
> @@ -2343,7 +2344,7 @@ static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
>  	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>  
>  	i915_gem_object_unpin_fence(obj);
> -	i915_gem_object_unpin_from_display_plane(obj);
> +	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
>  }
>  
>  /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 823d1d9..dd92122 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  	if (ret != 0)
>  		return ret;
>  
> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> +	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL,
> +						   &i915_ggtt_view_normal);
>  	if (ret != 0)
>  		return ret;
>  


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

Re: [PATCH 2/7] drm/i915: Use GGTT view when (un)pinning objects to planes

[Daniel Vetter]

On Wed, Mar 18, 2015 at 03:52:31PM +0200, Joonas Lahtinen wrote:
> On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> > @@ -3993,7 +3994,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  	 * (e.g. libkms for the bootup splash), we have to ensure that we
> >  	 * always use map_and_fenceable for all scanout buffers.
> >  	 */
> > -	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> > +	ret = i915_gem_object_ggtt_pin(obj, view, alignment,
> > +				       view->type == I915_GGTT_VIEW_NORMAL ?
> > +				       PIN_MAPPABLE : 0);
> 
> I'm slightly concerned about making an assumption that other but normal
> views need not to be mappable (when none are defined). As discussed in
> IRC, this should be moved later into the series when we actually know
> about the other views.

Just an aside: As soon as we have partial ggtt views we don't need to pin
anything display related as mappable any more - we only do this to make
sure we can serve any gtt mmap faults on frontbuffers, just in case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
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: Pass in plane state when (un)pinning frame buffers

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Plane state carries the rotation information which is needed for determining
> the appropriate GGTT view type.
> 
> This just adds the parameter with the actual usage coming in future patches.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 16f3443..862aa46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2252,6 +2252,7 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
>  int
>  intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			   struct drm_framebuffer *fb,
> +			   const struct drm_plane_state *plane_state,

Matter of taste, but I would have added the argument right before plane
argument.

>  			   struct intel_engine_cs *pipelined)
>  {
>  	struct drm_device *dev = fb->dev;
> @@ -2339,8 +2340,11 @@ err_interruptible:
>  	return ret;
>  }
>  
> -static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
> +static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> +			       const struct drm_plane_state *plane_state)
>  {
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
>  	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>  
>  	i915_gem_object_unpin_fence(obj);
> @@ -9277,7 +9281,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
> +	intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state);
>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
>  
>  	intel_fbc_update(dev);
> @@ -9985,7 +9989,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		ring = &dev_priv->ring[RCS];
>  	}
>  
> -	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring);
> +	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
> +					 crtc->primary->state, ring);
>  	if (ret)
>  		goto cleanup_pending;
>  
> @@ -10025,7 +10030,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	return 0;
>  
>  cleanup_unpin:
> -	intel_unpin_fb_obj(obj);
> +	intel_unpin_fb_obj(fb, crtc->primary->state);
>  cleanup_pending:
>  	atomic_dec(&intel_crtc->unpin_work_count);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -11981,7 +11986,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		if (ret)
>  			DRM_DEBUG_KMS("failed to attach phys object\n");
>  	} else {
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL);
>  	}
>  
>  	if (ret == 0)
> @@ -12013,7 +12018,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
>  	    !INTEL_INFO(dev)->cursor_needs_physical) {
>  		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(obj);
> +		intel_unpin_fb_obj(fb, old_state);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  }
> @@ -13906,6 +13911,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  
>  		if (intel_pin_and_fence_fb_obj(c->primary,
>  					       c->primary->fb,
> +					       c->primary->state,
>  					       NULL)) {
>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>  				  to_intel_crtc(c)->pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5254540..3721878 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -962,6 +962,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct intel_load_detect_pipe *old);
>  int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			       struct drm_framebuffer *fb,
> +			       const struct drm_plane_state *plane_state,
>  			       struct intel_engine_cs *pipelined);
>  struct drm_framebuffer *
>  __intel_framebuffer_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 757c0d2..4e7e7da 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -151,7 +151,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	}
>  
>  	/* Flush everything out, we'll be doing GTT only from now on */
> -	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
> +	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
>  		goto out_fb;


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

Re: [PATCH 4/7] drm/i915: Helper function to determine GGTT view from plane state

[Joonas Lahtinen]

I'd fix the below code style corrections, the functionality is ok.

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> For now only default implementation defaulting to normal view.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 862aa46..fe11e99 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2249,6 +2249,16 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
>  					       fb_format_modifier));
>  }
>  
> +static
> +int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,

Rather make this as it's elsewhere (static and return type on the same
line):
+static int
+intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,

> +			    struct drm_framebuffer *fb,
> +			    const struct drm_plane_state *plane_state)
> +{
> +	*view = i915_ggtt_view_normal;
> +
> +	return 0;
> +}
> +
>  int
>  intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			   struct drm_framebuffer *fb,
> @@ -2258,6 +2268,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	struct drm_device *dev = fb->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct i915_ggtt_view view;
>  	u32 alignment;
>  	int ret;
>  
> @@ -2294,6 +2305,10 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
> +	if (ret < 0)

Why not just "if (ret)" for consistency?

> +		return ret;
> +
>  	/* Note that the w/a also requires 64 PTE of padding following the
>  	 * bo. We currently fill all unused PTE with the shadow page and so
>  	 * we should always have valid PTE following the scanout preventing
> @@ -2313,7 +2328,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  
>  	dev_priv->mm.interruptible = false;
>  	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
> -						   &i915_ggtt_view_normal);
> +						   &view);
>  	if (ret)
>  		goto err_interruptible;
>  
> @@ -2333,7 +2348,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	return 0;
>  
>  err_unpin:
> -	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
> +	i915_gem_object_unpin_from_display_plane(obj, &view);
>  err_interruptible:
>  	dev_priv->mm.interruptible = true;
>  	intel_runtime_pm_put(dev_priv);
> @@ -2344,11 +2359,16 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>  			       const struct drm_plane_state *plane_state)
>  {
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct i915_ggtt_view view;
> +	int ret;
>  
>  	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>  
> +	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
> +	WARN_ONCE(ret < 0, "Couldn't get view from plane state!");

Again, just WARN_ONCE(ret, ...) as the function doesn't have need to
return any positive integer result.

> +
>  	i915_gem_object_unpin_fence(obj);
> -	i915_gem_object_unpin_from_display_plane(obj, &i915_ggtt_view_normal);
> +	i915_gem_object_unpin_from_display_plane(obj, &view);
>  }
>  
>  /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel


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

Re: [PATCH 5/7] drm/i915/skl: Support secondary (rotated) frame buffer mapping

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 90/270 rotated scanout needs a rotated GTT view of the framebuffer.
> 
> This is put in a separate VMA with a dedicated ggtt view and wired such that
> it is created when a framebuffer is pinned to a 90/270 rotated plane.
> 
> Rotation is only possible with Yb/Yf buffers and error is propagated to
> user space in case of a mismatch.
> 
> Special rotated page view is constructed at the VMA creation time by
> borrowing the DMA addresses from obj->pages.
> 
> v2:
>     * Do not bother with pages for rotated sg list, just populate the DMA
>       addresses. (Daniel Vetter)
>     * Checkpatch cleanup.
> 
> v3:
>     * Rebased on top of new plane handling (create rotated mapping when
>       setting the rotation property).
>     * Unpin rotated VMA on unpinning from display plane.
>     * Simplify rotation check using bitwise AND. (Chris Wilson)
> 
> v4:
>     * Fix unpinning of optional rotated mapping so it is really considered
>       to be optional.
> 
> v5:
>    * Rebased for fb modifier changes.
>    * Rebased for atomic commit.
>    * Only pin needed view for display. (Ville Syrjälä, Daniel Vetter)
> 
> v6:
>    * Rebased after preparatory work has been extracted out. (Daniel Vetter)
> 
> v7:
>    * Slightly simplified tiling geometry calculation.
>    * Moved rotated GGTT view implementation into i915_gem_gtt.c (Daniel Vetter)
> 
> v8:
>    * Do not use i915_gem_obj_size to get object size since that actually
>      returns the size of an VMA which may not exist.
>    * Rebased for ggtt view changes.
> 
> For: VIZ-4726
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v4)
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c  | 113 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_gtt.h  |  12 ++++
>  drivers/gpu/drm/i915/intel_display.c |  29 ++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   4 ++
>  4 files changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f1b9ea6..5381ebf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2413,15 +2413,119 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
>  
>  }
>  
> +static
> +void rotate_pages(dma_addr_t *in, unsigned int width, unsigned int height,
> +		  struct sg_table *st)
> +{
> +	unsigned int column, row;
> +	unsigned int src_idx;
> +	struct scatterlist *sg = st->sgl;
> +
> +	st->nents = 0;
> +
> +	for (column = 0; column < width; column++) {
> +		src_idx = width * (height - 1) + column;
> +		for (row = 0; row < height; row++) {
> +			st->nents++;
> +			/* We don't need the pages, but need to initialize
> +			 * the entries so the sg list can be happily traversed.
> +			 * The only thing we need are DMA addresses.
> +			 */
> +			sg_set_page(sg, NULL, PAGE_SIZE, 0);
> +			sg_dma_address(sg) = in[src_idx];
> +			sg_dma_len(sg) = PAGE_SIZE;
> +			sg = sg_next(sg);
> +			src_idx -= width;
> +		}
> +	}
> +}
> +
> +static
> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
> +					   struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
> +	unsigned long size, pages, rot_pages;
> +	struct sg_page_iter sg_iter;
> +	unsigned long i;
> +	dma_addr_t *page_addr_list;
> +	struct sg_table *st;
> +	unsigned int tile_pitch, tile_height;
> +	unsigned int width_pages, height_pages;
> +	int ret = ENOMEM;
> +
> +	pages = obj->base.size / PAGE_SIZE;
> +
> +	/* Calculate tiling geometry. */
> +	tile_height = intel_tile_height(dev, rot_info->pixel_format,
> +					rot_info->fb_modifier);
> +	tile_pitch = PAGE_SIZE / tile_height;
> +	width_pages = DIV_ROUND_UP(rot_info->pitch, tile_pitch);
> +	height_pages = DIV_ROUND_UP(rot_info->height, tile_height);
> +	rot_pages = width_pages * height_pages;
> +	size = rot_pages * PAGE_SIZE;
> +
> +	/* Allocate a temporary list of source pages for random access. */
> +	page_addr_list = drm_malloc_ab(pages, sizeof(dma_addr_t));
> +	if (!page_addr_list)
> +		return ERR_PTR(ret);
> +
> +	/* Allocate target SG list. */
> +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		goto err_st_alloc;
> +
> +	ret = sg_alloc_table(st, rot_pages, GFP_KERNEL);
> +	if (ret)
> +		goto err_sg_alloc;
> +
> +	/* Populate source page list from the object. */
> +	i = 0;
> +	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> +		page_addr_list[i] = sg_page_iter_dma_address(&sg_iter);
> +		i++;
> +	}
> +
> +	/* Rotate the pages. */
> +	rotate_pages(page_addr_list, width_pages, height_pages, st);
> +
> +	DRM_DEBUG_KMS(
> +		      "Created rotated page mapping for object size %lu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages).\n",
> +		      size, rot_info->pitch, rot_info->height,
> +		      rot_info->pixel_format, width_pages, height_pages,
> +		      rot_pages);
> +
> +	drm_free_large(page_addr_list);
> +
> +	return st;
> +
> +err_sg_alloc:
> +	kfree(st);
> +err_st_alloc:
> +	drm_free_large(page_addr_list);
> +
> +	DRM_DEBUG_KMS(
> +		      "Failed to create rotated mapping for object size %lu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %lu pages)\n",
> +		      size, ret, rot_info->pitch, rot_info->height,
> +		      rot_info->pixel_format, width_pages, height_pages,
> +		      rot_pages);
> +	return ERR_PTR(ret);
> +}
>  
>  static inline
>  int i915_get_ggtt_vma_pages(struct i915_vma *vma)

Same rant about function signatures as on earlier patch, put all on the
same line like most of new the code has it.

>  {
> +	int ret = 0;
> +
>  	if (vma->ggtt_view.pages)
>  		return 0;
>  
>  	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>  		vma->ggtt_view.pages = vma->obj->pages;
> +	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
> +		vma->ggtt_view.pages =
> +			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
>  	else
>  		WARN_ONCE(1, "GGTT view %u not implemented!\n",
>  			  vma->ggtt_view.type);
> @@ -2429,10 +2533,15 @@ int i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  	if (!vma->ggtt_view.pages) {
>  		DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
>  			  vma->ggtt_view.type);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +	} else if (IS_ERR(vma->ggtt_view.pages)) {
> +		ret = PTR_ERR(vma->ggtt_view.pages);
> +		vma->ggtt_view.pages = NULL;
> +		DRM_ERROR("Failed to get pages for VMA view type %u (%d)!\n",
> +			  vma->ggtt_view.type, ret);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index c9e93f5..a61e8dd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -111,12 +111,24 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>  
>  enum i915_ggtt_view_type {
>  	I915_GGTT_VIEW_NORMAL = 0,
> +	I915_GGTT_VIEW_ROTATED
> +};
> +
> +struct intel_rotation_info {
> +	unsigned int height;
> +	unsigned int pitch;
> +	uint32_t pixel_format;
> +	uint64_t fb_modifier;
>  };
>  
>  struct i915_ggtt_view {
>  	enum i915_ggtt_view_type type;
>  
>  	struct sg_table *pages;
> +
> +	union {
> +		struct intel_rotation_info rotation_info;
> +	};

In preparation for the memcmp way of comparing views, I would move this
be before the variable struct parts (namely sg_table *pages), and also
wrap it once more so the result would be like this:

[snip]
enum i915_ggtt_view_type type;

union {
	struct {
		struct intel_rotation_info info;
	} rotated;
	struct {
		...
	} partial;
};

// private bits go here, to be wrapped in their struct with view
// type comparing patches

struct sg_table *pages;
[snip]

That way it's clear which view owns what.

>  };
>  
>  extern const struct i915_ggtt_view i915_ggtt_view_normal;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe11e99..c2c3a76 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
>  	return false;
>  }
>  
> -static unsigned int
> +unsigned int
>  intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
>  		  uint64_t fb_format_modifier)
>  {
> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>  			    struct drm_framebuffer *fb,
>  			    const struct drm_plane_state *plane_state)
>  {
> +	struct intel_rotation_info *info = &view->rotation_info;
> +	static const struct i915_ggtt_view rotated_view =
> +				{ .type = I915_GGTT_VIEW_ROTATED };
> +
>  	*view = i915_ggtt_view_normal;
>  
> -	return 0;
> +	if (!plane_state)
> +		return 0;
> +
> +	if (!(plane_state->rotation &
> +	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
> +		return 0;

Should the rotation checking helper introduced in previous patch be used
here too?

> +
> +	*view = rotated_view;
> +
> +	info->height = fb->height;
> +	info->pixel_format = fb->pixel_format;
> +	info->pitch = fb->pitches[0];
> +	info->fb_modifier = fb->modifier[0];
> +
> +	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> +	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> +		DRM_DEBUG_KMS(
> +			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> +		return -EINVAL;
> +	}
> +
> +	return 1;
>  }
>  
>  int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3721878..53a1372 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -987,6 +987,10 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
>  				    struct drm_property *property,
>  				    uint64_t val);
>  
> +unsigned int
> +intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> +		  uint64_t fb_format_modifier);
> +
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,


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

Re: [PATCH 6/7] drm/i915/skl: Query display address through a wrapper

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Need to do this in order to support 90/270 rotated display.
> 
> v2: Pass in drm_plane instead of plane index to intel_obj_display_address.
> 
> v3:
>     * Renamed intel_obj_display_address to intel_plane_obj_offset.
>       (Chris Wilson)
>     * Simplified rotation check to bitwise AND. (Chris Wilson)
> 
> v4:
>     * Extracted 90/270 rotation check into a helper function. (Michel Thierry)
> 
> v5:
>     * Rebased for ggtt view changes.
> 
> For: VIZ-4545
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v5)

> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  9 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |  5 ++++-
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c2c3a76..9af73b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2263,8 +2263,7 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>  	if (!plane_state)
>  		return 0;
>  
> -	if (!(plane_state->rotation &
> -	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
> +	if (!intel_rotation_90_or_270(plane_state->rotation))
>  		return 0;
>  
>  	*view = rotated_view;
> @@ -2862,6 +2861,17 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>  	}
>  }
>  
> +unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> +				     struct drm_i915_gem_object *obj)
> +{
> +	enum i915_ggtt_view_type view = I915_GGTT_VIEW_NORMAL;
> +
> +	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
> +		view = I915_GGTT_VIEW_ROTATED;
> +
> +	return i915_gem_obj_ggtt_offset_view(obj, view);
> +}
> +
>  static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					 struct drm_framebuffer *fb,
>  					 int x, int y)
> @@ -2872,6 +2882,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_i915_gem_object *obj;
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl, stride_div;
> +	unsigned long surf_addr;
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -2938,16 +2949,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	obj = intel_fb_obj(fb);
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
> +	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> -
>  	I915_WRITE(PLANE_POS(pipe, 0), 0);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
>  	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), fb->pitches[0] / stride_div);
> -	I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
> +	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>  
>  	POSTING_READ(PLANE_SURF(pipe, 0));
>  }
> @@ -10039,8 +10050,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (ret)
>  		goto cleanup_pending;
>  
> -	work->gtt_offset =
> -		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +	work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary), obj)
> +						  + intel_crtc->dspaddr_offset;
>  
>  	if (use_mmio_flip(ring, obj)) {
>  		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 53a1372..ae28fb4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -991,6 +991,12 @@ unsigned int
>  intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
>  		  uint64_t fb_format_modifier);
>  
> +static inline bool
> +intel_rotation_90_or_270(unsigned int rotation)
> +{
> +	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> +}
> +
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
> @@ -1045,6 +1051,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
> +unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> +				     struct drm_i915_gem_object *obj);
> +
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>  bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a828736..d960572 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -191,6 +191,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl, stride_div;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	unsigned long surf_addr;
>  
>  	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
>  
> @@ -280,12 +281,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> +
>  	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>  	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);
> -	I915_WRITE(PLANE_SURF(pipe, plane), i915_gem_obj_ggtt_offset(obj));
> +	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
>  }
>  


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

Re: [PATCH 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[Joonas Lahtinen]

On ti, 2015-03-17 at 15:45 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> v2: Pass in rotation info to sprite plane updates as well.
> 
> v3: Use helper to determine 90/270 rotation. (Michel Thierry)
> 
> v4: Rebased for fb modifiers and atomic changes.
> 
> For: VIZ-4546
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v4)

> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 18 +++++++++++++++++-
>  drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
>  4 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9af73b1..19bec33 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,6 +11994,28 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  }
>  
>  /**
> + * intel_wm_need_update - Check whether watermarks need updating
> + * @plane: drm plane
> + * @state: new plane state
> + *
> + * Check current plane state versus the new one to determine whether
> + * watermarks need to be recalculated.
> + *
> + * Returns true or false.
> + */
> +bool intel_wm_need_update(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	/* Update watermarks on tiling changes. */
> +	if (!plane->state->fb || !state->fb ||
> +	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> +	    plane->state->rotation != state->rotation)
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
>   * intel_prepare_plane_fb - Prepare fb for usage on plane
>   * @plane: drm plane to prepare for
>   * @fb: framebuffer to prepare for presentation
> @@ -12139,10 +12161,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  
>  		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])
> +		if (intel_wm_need_update(plane, &state->base))
>  			intel_crtc->atomic.update_wm = true;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ae28fb4..a7ecf95 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 {
>  	bool enabled;
>  	bool scaled;
>  	u64 tiling;
> +	unsigned int rotation;
>  };
>  
>  struct intel_plane {
> @@ -997,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
>  	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>  }
>  
> +bool intel_wm_need_update(struct drm_plane *plane,
> +			  struct drm_plane_state *state);
> +
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 288c9d2..4e080a6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2840,6 +2840,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		}
>  		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].rotation = crtc->primary->state->rotation;
>  
>  		fb = crtc->cursor->state->fb;
>  		if (fb) {
> @@ -2897,7 +2898,21 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  
>  	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
>  	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> -		uint32_t y_tile_minimum = plane_blocks_per_line * 4;
> +		uint32_t min_scanlines = 4;
> +		uint32_t y_tile_minimum;
> +		if (intel_rotation_90_or_270(p_params->rotation)) {
> +			switch (p_params->bytes_per_pixel) {
> +			case 1:
> +				min_scanlines = 16;
> +				break;
> +			case 2:
> +				min_scanlines = 8;
> +				break;
> +			case 8:
> +				WARN(1, "Unsupported pixel depth for rotation");
> +			};
> +		}
> +		y_tile_minimum = plane_blocks_per_line * min_scanlines;
>  		selected_result = max(method2, y_tile_minimum);
>  	} else {
>  		if ((ddb_allocation / plane_blocks_per_line) >= 1)
> @@ -3357,6 +3372,7 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
>  	 */
>  	if (fb)
>  		intel_plane->wm.tiling = fb->modifier[0];
> +	intel_plane->wm.rotation = plane->state->rotation;
>  
>  	skl_update_wm(crtc);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d960572..b919b0a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1260,10 +1260,7 @@ 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])
> +		if (intel_wm_need_update(plane, &state->base))
>  			intel_crtc->atomic.update_wm = true;
>  	}
>  


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

Re: [PATCH 5/7] drm/i915/skl: Support secondary (rotated) frame buffer mapping

[Tvrtko Ursulin]

Hi,

On 03/19/2015 01:02 PM, Joonas Lahtinen wrote:
>>   static inline
>>   int i915_get_ggtt_vma_pages(struct i915_vma *vma)
>
> Same rant about function signatures as on earlier patch, put all on the
> same line like most of new the code has it.

Ok.

>>   struct i915_ggtt_view {
>>   	enum i915_ggtt_view_type type;
>>
>>   	struct sg_table *pages;
>> +
>> +	union {
>> +		struct intel_rotation_info rotation_info;
>> +	};
>
> In preparation for the memcmp way of comparing views, I would move this
> be before the variable struct parts (namely sg_table *pages), and also
> wrap it once more so the result would be like this:
>
> [snip]
> enum i915_ggtt_view_type type;
>
> union {
> 	struct {
> 		struct intel_rotation_info info;
> 	} rotated;
> 	struct {
> 		...
> 	} partial;
> };
>
> // private bits go here, to be wrapped in their struct with view
> // type comparing patches
>
> struct sg_table *pages;
> [snip]
>
> That way it's clear which view owns what.

Hm, rotation info is not considered in comparing views, it is just a 
bucket of data passed around between layers. So I suppose private data 
under your design. Since there is no private union yet, maybe do this later?

>>   extern const struct i915_ggtt_view i915_ggtt_view_normal;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index fe11e99..c2c3a76 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
>>   	return false;
>>   }
>>
>> -static unsigned int
>> +unsigned int
>>   intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
>>   		  uint64_t fb_format_modifier)
>>   {
>> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>>   			    struct drm_framebuffer *fb,
>>   			    const struct drm_plane_state *plane_state)
>>   {
>> +	struct intel_rotation_info *info = &view->rotation_info;
>> +	static const struct i915_ggtt_view rotated_view =
>> +				{ .type = I915_GGTT_VIEW_ROTATED };
>> +
>>   	*view = i915_ggtt_view_normal;
>>
>> -	return 0;
>> +	if (!plane_state)
>> +		return 0;
>> +
>> +	if (!(plane_state->rotation &
>> +	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
>> +		return 0;
>
> Should the rotation checking helper introduced in previous patch be used
> here too?

It's the other way round, following patch adds the helper and replaces 
this with a call to it.

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: Support secondary (rotated) frame buffer mapping

[Joonas Lahtinen]

On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote:
> Hi,
> 
> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote:
> >>   static inline
> >>   int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >
> > Same rant about function signatures as on earlier patch, put all on the
> > same line like most of new the code has it.
> 
> Ok.
> 
> >>   struct i915_ggtt_view {
> >>   	enum i915_ggtt_view_type type;
> >>
> >>   	struct sg_table *pages;
> >> +
> >> +	union {
> >> +		struct intel_rotation_info rotation_info;
> >> +	};
> >
> > In preparation for the memcmp way of comparing views, I would move this
> > be before the variable struct parts (namely sg_table *pages), and also
> > wrap it once more so the result would be like this:
> >
> > [snip]
> > enum i915_ggtt_view_type type;
> >
> > union {
> > 	struct {
> > 		struct intel_rotation_info info;
> > 	} rotated;
> > 	struct {
> > 		...
> > 	} partial;
> > };
> >
> > // private bits go here, to be wrapped in their struct with view
> > // type comparing patches
> >
> > struct sg_table *pages;
> > [snip]
> >
> > That way it's clear which view owns what.
> 
> Hm, rotation info is not considered in comparing views, it is just a 
> bucket of data passed around between layers. So I suppose private data 
> under your design. Since there is no private union yet, maybe do this later?

Why not? Isn't a 270 degree rotated view substantially different from a
90 degree rotated view (even when the difference technically is just
some bit flip somewhere else).

At least I would be pretty upset if I was returned the address for 90
degree rotated view when I wanted 270 rotated. If multiple rotated views
are not possible, then it is again an implicit thing.

There are quite a lot of hardware constraints like this that appear in
the code implicitly, which IMHO makes the code hard to follow at times.
So I'd try to make it more explicit that the views are not the same,
there just can be one rotated view at a time (if that is the case).

> 
> >>   extern const struct i915_ggtt_view i915_ggtt_view_normal;
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index fe11e99..c2c3a76 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
> >>   	return false;
> >>   }
> >>
> >> -static unsigned int
> >> +unsigned int
> >>   intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> >>   		  uint64_t fb_format_modifier)
> >>   {
> >> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
> >>   			    struct drm_framebuffer *fb,
> >>   			    const struct drm_plane_state *plane_state)
> >>   {
> >> +	struct intel_rotation_info *info = &view->rotation_info;
> >> +	static const struct i915_ggtt_view rotated_view =
> >> +				{ .type = I915_GGTT_VIEW_ROTATED };
> >> +
> >>   	*view = i915_ggtt_view_normal;
> >>
> >> -	return 0;
> >> +	if (!plane_state)
> >> +		return 0;
> >> +
> >> +	if (!(plane_state->rotation &
> >> +	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
> >> +		return 0;
> >
> > Should the rotation checking helper introduced in previous patch be used
> > here too?
> 
> It's the other way round, following patch adds the helper and replaces 
> this with a call to it.
> 

Yes, my bad, I got:)

Regards, Joonas

> 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: Support secondary (rotated) frame buffer mapping

[Tvrtko Ursulin]

On 03/20/2015 12:01 PM, Joonas Lahtinen wrote:
> On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote:
>>>>    static inline
>>>>    int i915_get_ggtt_vma_pages(struct i915_vma *vma)
>>>
>>> Same rant about function signatures as on earlier patch, put all on the
>>> same line like most of new the code has it.
>>
>> Ok.
>>
>>>>    struct i915_ggtt_view {
>>>>    	enum i915_ggtt_view_type type;
>>>>
>>>>    	struct sg_table *pages;
>>>> +
>>>> +	union {
>>>> +		struct intel_rotation_info rotation_info;
>>>> +	};
>>>
>>> In preparation for the memcmp way of comparing views, I would move this
>>> be before the variable struct parts (namely sg_table *pages), and also
>>> wrap it once more so the result would be like this:
>>>
>>> [snip]
>>> enum i915_ggtt_view_type type;
>>>
>>> union {
>>> 	struct {
>>> 		struct intel_rotation_info info;
>>> 	} rotated;
>>> 	struct {
>>> 		...
>>> 	} partial;
>>> };
>>>
>>> // private bits go here, to be wrapped in their struct with view
>>> // type comparing patches
>>>
>>> struct sg_table *pages;
>>> [snip]
>>>
>>> That way it's clear which view owns what.
>>
>> Hm, rotation info is not considered in comparing views, it is just a
>> bucket of data passed around between layers. So I suppose private data
>> under your design. Since there is no private union yet, maybe do this later?
>
> Why not? Isn't a 270 degree rotated view substantially different from a
> 90 degree rotated view (even when the difference technically is just
> some bit flip somewhere else).
>
> At least I would be pretty upset if I was returned the address for 90
> degree rotated view when I wanted 270 rotated. If multiple rotated views
> are not possible, then it is again an implicit thing.
>
> There are quite a lot of hardware constraints like this that appear in
> the code implicitly, which IMHO makes the code hard to follow at times.
> So I'd try to make it more explicit that the views are not the same,
> there just can be one rotated view at a time (if that is the case).

90 and 270 views are indeed the same page layout - same address for 
scanout. And there can only be one such VMA for an object at a time.

But how this mapping needs to look like is determined by more than the 
object itself - framebuffer geometry defines it. The private data in the 
view is used to transfer that meta-data so the GTT core can build the 
appropriate view.

That was my argument in fact for not putting the page shuffling bit in 
i915_gem_gtt.c since it is really display engine ownership.

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: Support secondary (rotated) frame buffer mapping

[Joonas Lahtinen]

Hi,

On pe, 2015-03-20 at 12:11 +0000, Tvrtko Ursulin wrote:
> On 03/20/2015 12:01 PM, Joonas Lahtinen wrote:
> > On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote:
> >> Hi,
> >>
> >> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote:
> >>>>    static inline
> >>>>    int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >>>
> >>> Same rant about function signatures as on earlier patch, put all on the
> >>> same line like most of new the code has it.
> >>
> >> Ok.
> >>
> >>>>    struct i915_ggtt_view {
> >>>>    	enum i915_ggtt_view_type type;
> >>>>
> >>>>    	struct sg_table *pages;
> >>>> +
> >>>> +	union {
> >>>> +		struct intel_rotation_info rotation_info;
> >>>> +	};
> >>>
> >>> In preparation for the memcmp way of comparing views, I would move this
> >>> be before the variable struct parts (namely sg_table *pages), and also
> >>> wrap it once more so the result would be like this:
> >>>
> >>> [snip]
> >>> enum i915_ggtt_view_type type;
> >>>
> >>> union {
> >>> 	struct {
> >>> 		struct intel_rotation_info info;
> >>> 	} rotated;
> >>> 	struct {
> >>> 		...
> >>> 	} partial;
> >>> };
> >>>
> >>> // private bits go here, to be wrapped in their struct with view
> >>> // type comparing patches
> >>>
> >>> struct sg_table *pages;
> >>> [snip]
> >>>
> >>> That way it's clear which view owns what.
> >>
> >> Hm, rotation info is not considered in comparing views, it is just a
> >> bucket of data passed around between layers. So I suppose private data
> >> under your design. Since there is no private union yet, maybe do this later?
> >
> > Why not? Isn't a 270 degree rotated view substantially different from a
> > 90 degree rotated view (even when the difference technically is just
> > some bit flip somewhere else).
> >
> > At least I would be pretty upset if I was returned the address for 90
> > degree rotated view when I wanted 270 rotated. If multiple rotated views
> > are not possible, then it is again an implicit thing.
> >
> > There are quite a lot of hardware constraints like this that appear in
> > the code implicitly, which IMHO makes the code hard to follow at times.
> > So I'd try to make it more explicit that the views are not the same,
> > there just can be one rotated view at a time (if that is the case).
> 
> 90 and 270 views are indeed the same page layout - same address for 
> scanout. And there can only be one such VMA for an object at a time.
> 
> But how this mapping needs to look like is determined by more than the 
> object itself - framebuffer geometry defines it. The private data in the 
> view is used to transfer that meta-data so the GTT core can build the 
> appropriate view.
> 

Right, I think I understand your viewpoint now. I would still prefer it
to be even more explicit like I915_GGTT_VIEW_Yf_SCANOUT, because what it
really does is rearranges the pages to layout suitable for rotated
scanout, not making the view rotated in itself.

And in that case it would not be in the memcmp range, like it is not
currently.

Regards, Joonas

> That was my argument in fact for not putting the page shuffling bit in 
> i915_gem_gtt.c since it is really display engine ownership.
> 
> 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: Support secondary (rotated) frame buffer mapping

[Tvrtko Ursulin]

On 03/20/2015 01:31 PM, Joonas Lahtinen wrote:

[snip]

>>>> Hm, rotation info is not considered in comparing views, it is just a
>>>> bucket of data passed around between layers. So I suppose private data
>>>> under your design. Since there is no private union yet, maybe do this later?
>>>
>>> Why not? Isn't a 270 degree rotated view substantially different from a
>>> 90 degree rotated view (even when the difference technically is just
>>> some bit flip somewhere else).
>>>
>>> At least I would be pretty upset if I was returned the address for 90
>>> degree rotated view when I wanted 270 rotated. If multiple rotated views
>>> are not possible, then it is again an implicit thing.
>>>
>>> There are quite a lot of hardware constraints like this that appear in
>>> the code implicitly, which IMHO makes the code hard to follow at times.
>>> So I'd try to make it more explicit that the views are not the same,
>>> there just can be one rotated view at a time (if that is the case).
>>
>> 90 and 270 views are indeed the same page layout - same address for
>> scanout. And there can only be one such VMA for an object at a time.
>>
>> But how this mapping needs to look like is determined by more than the
>> object itself - framebuffer geometry defines it. The private data in the
>> view is used to transfer that meta-data so the GTT core can build the
>> appropriate view.
>>
>
> Right, I think I understand your viewpoint now. I would still prefer it
> to be even more explicit like I915_GGTT_VIEW_Yf_SCANOUT, because what it
> really does is rearranges the pages to layout suitable for rotated
> scanout, not making the view rotated in itself.

Ha, not sure what you mean, but Yf is definitely not the right name for 
it since it is a different thing altogether.

It is rotated in a way that tiles (==pages) are rotated by 90 degrees. 
But content within the tile is not (display engine handles that bit). In 
your mind that would maybe be 
I915_GGTT_VIEW_ROTATED_PAGES_FOR_ROTATED_SCANOUT? :) I am guessing only, 
but I still think I915_GGTT_VIEW_ROTATED is fine.

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: Take 90/270 rotation into account in watermark calculations

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6028
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  275/275              275/275
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                                  339/339              339/339
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  310/310              310/310
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
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 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[Tvrtko Ursulin]

On 03/23/2015 02:12 PM, Daniel Vetter wrote:
> On Mon, Mar 23, 2015 at 11:10:38AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> v2: Pass in rotation info to sprite plane updates as well.
>>
>> v3: Use helper to determine 90/270 rotation. (Michel Thierry)
>>
>> v4: Rebased for fb modifiers and atomic changes.
>>
>> For: VIZ-4546
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Merged the entire series, thanks. I noticed though that we don't yet
> expose 90/270 rotation to userspace, I guess that'll happen when the
> tests/userspace are ready?

Thank you!

And correct - Sonika has a patch series which needs to be rebased on top 
of this and which actually adds plane programming and exposes the plane 
properties. Plus kms_rotation_crc extensions of course.

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: Take 90/270 rotation into account in watermark calculations

[Daniel Vetter]

On Mon, Mar 23, 2015 at 11:10:38AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> v2: Pass in rotation info to sprite plane updates as well.
> 
> v3: Use helper to determine 90/270 rotation. (Michel Thierry)
> 
> v4: Rebased for fb modifiers and atomic changes.
> 
> For: VIZ-4546
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Merged the entire series, thanks. I noticed though that we don't yet
expose 90/270 rotation to userspace, I guess that'll happen when the
tests/userspace are ready?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[Tvrtko Ursulin]

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

v2: Pass in rotation info to sprite plane updates as well.

v3: Use helper to determine 90/270 rotation. (Michel Thierry)

v4: Rebased for fb modifiers and atomic changes.

For: VIZ-4546
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 18 +++++++++++++++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
 4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d1f9099..35cdb48 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12034,6 +12034,28 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 }
 
 /**
+ * intel_wm_need_update - Check whether watermarks need updating
+ * @plane: drm plane
+ * @state: new plane state
+ *
+ * Check current plane state versus the new one to determine whether
+ * watermarks need to be recalculated.
+ *
+ * Returns true or false.
+ */
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	/* Update watermarks on tiling changes. */
+	if (!plane->state->fb || !state->fb ||
+	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
+	    plane->state->rotation != state->rotation)
+		return true;
+
+	return false;
+}
+
+/**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
  * @fb: framebuffer to prepare for presentation
@@ -12179,10 +12201,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 		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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index db0e7bf..811a1db 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 {
 	bool enabled;
 	bool scaled;
 	u64 tiling;
+	unsigned int rotation;
 };
 
 struct intel_plane {
@@ -994,6 +995,9 @@ intel_rotation_90_or_270(unsigned int rotation)
 	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
 }
 
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state);
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e18f0fd..753a3af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2840,6 +2840,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		}
 		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].rotation = crtc->primary->state->rotation;
 
 		fb = crtc->cursor->state->fb;
 		if (fb) {
@@ -2897,7 +2898,21 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
 	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t y_tile_minimum = plane_blocks_per_line * 4;
+		uint32_t min_scanlines = 4;
+		uint32_t y_tile_minimum;
+		if (intel_rotation_90_or_270(p_params->rotation)) {
+			switch (p_params->bytes_per_pixel) {
+			case 1:
+				min_scanlines = 16;
+				break;
+			case 2:
+				min_scanlines = 8;
+				break;
+			case 8:
+				WARN(1, "Unsupported pixel depth for rotation");
+			};
+		}
+		y_tile_minimum = plane_blocks_per_line * min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3357,6 +3372,7 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	if (fb)
 		intel_plane->wm.tiling = fb->modifier[0];
+	intel_plane->wm.rotation = plane->state->rotation;
 
 	skl_update_wm(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 005a6fd..f41e872 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1039,10 +1039,7 @@ 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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 
 		if (!state->visible) {
-- 
2.3.2

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

Re: [PATCH 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5896
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  275/275              275/275
ILK                                  307/307              307/307
SNB                 -1              284/284              283/284
IVB                                  375/375              375/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                 -1              314/314              313/314
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt_gem_flink_bad-open      PASS(1)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(2)      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

[PATCH 7/7] drm/i915/skl: Take 90/270 rotation into account in watermark calculations

[Tvrtko Ursulin]

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

v2: Pass in rotation info to sprite plane updates as well.

v3: Use helper to determine 90/270 rotation. (Michel Thierry)

v4: Rebased for fb modifiers and atomic changes.

For: VIZ-4546
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> (v3)
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 18 +++++++++++++++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  5 +----
 4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c7ead35..6425f31 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11942,6 +11942,28 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 }
 
 /**
+ * intel_wm_need_update - Check whether watermarks need updating
+ * @plane: drm plane
+ * @state: new plane state
+ *
+ * Check current plane state versus the new one to determine whether
+ * watermarks need to be recalculated.
+ *
+ * Returns true or false.
+ */
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	/* Update watermarks on tiling changes. */
+	if (!plane->state->fb || !state->fb ||
+	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
+	    plane->state->rotation != state->rotation)
+		return true;
+
+	return false;
+}
+
+/**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
  * @fb: framebuffer to prepare for presentation
@@ -12087,10 +12109,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 		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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d57b9b7..d03d3bd 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 {
 	bool enabled;
 	bool scaled;
 	u64 tiling;
+	unsigned int rotation;
 };
 
 struct intel_plane {
@@ -993,6 +994,9 @@ intel_rotation_90_or_270(unsigned int rotation)
 	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
 }
 
+bool intel_wm_need_update(struct drm_plane *plane,
+			  struct drm_plane_state *state);
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e710b43..49e7d4e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2712,6 +2712,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		 */
 		if (fb)
 			p->plane[0].tiling = fb->modifier[0];
+		p->plane[0].rotation = crtc->primary->state->rotation;
 
 		p->cursor.enabled = true;
 		p->cursor.bytes_per_pixel = 4;
@@ -2761,7 +2762,21 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
 	    p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t y_tile_minimum = plane_blocks_per_line * 4;
+		uint32_t min_scanlines = 4;
+		uint32_t y_tile_minimum;
+		if (intel_rotation_90_or_270(p_params->rotation)) {
+			switch (p_params->bytes_per_pixel) {
+			case 1:
+				min_scanlines = 16;
+				break;
+			case 2:
+				min_scanlines = 8;
+				break;
+			case 8:
+				WARN(1, "Unsupported pixel depth for rotation");
+			};
+		}
+		y_tile_minimum = plane_blocks_per_line * min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3221,6 +3236,7 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 	 */
 	if (fb)
 		intel_plane->wm.tiling = fb->modifier[0];
+	intel_plane->wm.rotation = plane->state->rotation;
 
 	skl_update_wm(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index caa0bcc..addc90e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1260,10 +1260,7 @@ 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])
+		if (intel_wm_need_update(plane, &state->base))
 			intel_crtc->atomic.update_wm = true;
 	}
 
-- 
2.3.0

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