[PATCH v2 01/10] drm/i915: Merge of visible and !visible paths for primary planes

[PATCH v2 01/10] drm/i915: Merge of visible and !visible paths for primary planes

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Fold intel_pipe_set_base() in the update primary plane path merging
pieces of code that are common to both paths.

Basically the the pin/unpin procedures are the same for both paths
and some checks can also be shared (some of the were moved to the
check() stage)

v2: take Ville's comments:
	- remove unnecessary plane check
	- move mutex lock to inside the conditional
	- make the pin fail message a debug one
	- add a fixme for the fastboot hack
	- call intel_frontbuffer_flip() after FBC update

v3: take more Ville's comments:
	- fold update code under if (intel_crtc->active), and do the
	visible/!visible split inside.
	- check ret inside the same conditional we assign it

v4: don't use intel_enable_primary_hw_plane(), the primary_enabled
check inside will break page flips

v5: take more Ville's comments:
	- set primary_enabled to true and add BDW hack
	- unify if (old_fb) and if (old_fb != fb)

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 142 +++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 858011d..bef37dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11578,12 +11578,23 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	int ret;
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    false, true, &state->visible);
+	if (ret)
+		return ret;
+
+	/* no fb bound */
+	if (state->visible && !fb) {
+		DRM_ERROR("No FB bound\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int
@@ -11595,6 +11606,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *old_fb = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -11603,76 +11616,95 @@ intel_commit_primary_plane(struct drm_plane *plane,
 
 	intel_crtc_wait_for_pending_flips(crtc);
 
-	/*
-	 * If clipping results in a non-visible primary plane, we'll disable
-	 * the primary plane.  Note that this is a bit different than what
-	 * happens if userspace explicitly disables the plane by passing fb=0
-	 * because plane->fb still gets set and pinned.
-	 */
-	if (!state->visible) {
+	if (intel_crtc_has_pending_flip(crtc)) {
+		DRM_ERROR("pipe is still busy with an old pageflip\n");
+		return -EBUSY;
+	}
+
+	if (plane->fb != fb) {
 		mutex_lock(&dev->struct_mutex);
+		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		if (ret == 0)
+			i915_gem_track_fb(old_obj, obj,
+					  INTEL_FRONTBUFFER_PRIMARY(pipe));
+		mutex_unlock(&dev->struct_mutex);
+		if (ret != 0) {
+			DRM_DEBUG_KMS("pin & fence failed\n");
+			return ret;
+		}
+	}
+
+	crtc->primary->fb = fb;
+	crtc->x = src->x1;
+	crtc->y = src->y1;
 
+	intel_plane->crtc_x = state->orig_dst.x1;
+	intel_plane->crtc_y = state->orig_dst.y1;
+	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
+	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
+	intel_plane->src_x = state->orig_src.x1;
+	intel_plane->src_y = state->orig_src.y1;
+	intel_plane->src_w = drm_rect_width(&state->orig_src);
+	intel_plane->src_h = drm_rect_height(&state->orig_src);
+	intel_plane->obj = obj;
+
+	if (intel_crtc->active) {
 		/*
-		 * Try to pin the new fb first so that we can bail out if we
-		 * fail.
+		 * FBC does not work on some platforms for rotated
+		 * planes, so disable it when rotation is not 0 and
+		 * update it when rotation is set back to 0.
+		 *
+		 * FIXME: This is redundant with the fbc update done in
+		 * the primary plane enable function except that that
+		 * one is done too late. We eventually need to unify
+		 * this.
 		 */
-		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
-			if (ret) {
-				mutex_unlock(&dev->struct_mutex);
-				return ret;
-			}
+		if (intel_crtc->primary_enabled &&
+		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+		    dev_priv->fbc.plane == intel_crtc->plane &&
+		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
+			intel_disable_fbc(dev);
 		}
 
-		i915_gem_track_fb(old_obj, obj,
-				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-
-		if (intel_crtc->primary_enabled)
-			intel_disable_primary_hw_plane(plane, crtc);
+		if (state->visible) {
+			int was_enabled = intel_crtc->primary_enabled;
 
+			/* FIXME: kill this fastboot hack */
+			intel_update_pipe_size(intel_crtc);
 
-		if (plane->fb != fb)
-			if (plane->fb)
-				intel_unpin_fb_obj(old_obj);
+			intel_crtc->primary_enabled = true;
 
-		mutex_unlock(&dev->struct_mutex);
+			dev_priv->display.update_primary_plane(crtc, plane->fb,
+					crtc->x, crtc->y);
 
-	} else {
-		if (intel_crtc && intel_crtc->active &&
-		    intel_crtc->primary_enabled) {
+			if (IS_BROADWELL(dev) && was_enabled)
+				intel_wait_for_vblank(dev, intel_crtc->pipe);
+		} else {
 			/*
-			 * FBC does not work on some platforms for rotated
-			 * planes, so disable it when rotation is not 0 and
-			 * update it when rotation is set back to 0.
-			 *
-			 * FIXME: This is redundant with the fbc update done in
-			 * the primary plane enable function except that that
-			 * one is done too late. We eventually need to unify
-			 * this.
+			 * If clipping results in a non-visible primary plane,
+			 * we'll disable the primary plane.  Note that this is
+			 * a bit different than what happens if userspace
+			 * explicitly disables the plane by passing fb=0
+			 * because plane->fb still gets set and pinned.
 			 */
-			if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-			    dev_priv->fbc.plane == intel_crtc->plane &&
-			    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
-				intel_disable_fbc(dev);
-			}
+			intel_disable_primary_hw_plane(plane, crtc);
 		}
-		ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
-		if (ret)
-			return ret;
 
-		if (!intel_crtc->primary_enabled)
-			intel_enable_primary_hw_plane(plane, crtc);
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
+		mutex_lock(&dev->struct_mutex);
+		intel_update_fbc(dev);
+		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
-	intel_plane->obj = obj;
+	if (old_fb && old_fb != fb) {
+		if (intel_crtc->active)
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+		mutex_lock(&dev->struct_mutex);
+		intel_unpin_fb_obj(old_obj);
+		mutex_unlock(&dev->struct_mutex);
+	}
 
 	return 0;
 }
-- 
1.9.3

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

[PATCH v2 02/10] drm/i915: remove leftover from pre-universal planes days

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now that universal planes are in place we don't need this plane unref on
failures.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bef37dc..2ef1836 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8362,8 +8362,7 @@ static bool cursor_size_ok(struct drm_device *dev,
 /*
  * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
  *
- * Note that the object's reference will be consumed if the update fails.  If
- * the update succeeds, the reference of the old object (if any) will be
+ * If the update succeeds, the reference of the old object (if any) will be
  * consumed.
  */
 static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
@@ -8395,8 +8394,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	stride = roundup_pow_of_two(width) * 4;
 	if (obj->base.size < stride * height) {
 		DRM_DEBUG_KMS("buffer is too small\n");
-		ret = -ENOMEM;
-		goto fail;
+		return -ENOMEM;
 	}
 
 	/* we only need to pin inside GTT if cursor is non-phy */
@@ -8485,8 +8483,6 @@ fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
 	mutex_unlock(&dev->struct_mutex);
-fail:
-	drm_gem_object_unreference_unlocked(&obj->base);
 	return ret;
 }
 
-- 
1.9.3

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

[PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Move checks inside intel_crtc_cursor_set_obj() to
intel_check_cursor_plane(), we only use they there so move them out to
make the merge of intel_crtc_cursor_set_obj() into
intel_check_cursor_plane() easier.

This is another step toward the atomic modesetting support and unification
of plane operations such pin/unpin of fb objects on i915.

v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the
code

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ef1836..3f37e93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width, stride;
+	unsigned old_width;
 	uint32_t addr;
 	int ret;
 
@@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		goto finish;
 	}
 
-	/* Check for which cursor types we support */
-	if (!cursor_size_ok(dev, width, height)) {
-		DRM_DEBUG("Cursor dimension not supported\n");
-		return -EINVAL;
-	}
-
-	stride = roundup_pow_of_two(width) * 4;
-	if (obj->base.size < stride * height) {
-		DRM_DEBUG_KMS("buffer is too small\n");
-		return -ENOMEM;
-	}
-
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 
-		if (obj->tiling_mode) {
-			DRM_DEBUG_KMS("cursor cannot be tiled\n");
-			ret = -EINVAL;
-			goto fail_locked;
-		}
-
 		/*
 		 * Global gtt pte registers are special registers which actually
 		 * forward writes to a chunk of system memory. Which means that
@@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	int crtc_w, crtc_h;
+	unsigned stride;
+	int ret;
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    true, true, &state->visible);
+	if (ret)
+		return ret;
+
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj)
+		return 0;
+
+	if (fb == crtc->cursor->fb)
+		return 0;
+
+	/* Check for which cursor types we support */
+	crtc_w = drm_rect_width(&state->orig_dst);
+	crtc_h = drm_rect_height(&state->orig_dst);
+	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
+		DRM_DEBUG("Cursor dimension not supported\n");
+		return -EINVAL;
+	}
+
+	stride = roundup_pow_of_two(crtc_w) * 4;
+	if (obj->base.size < stride * crtc_h) {
+		DRM_DEBUG_KMS("buffer is too small\n");
+		return -ENOMEM;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
+		DRM_DEBUG_KMS("cursor cannot be tiled\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret;
 }
 
 static int
-- 
1.9.3

[PATCH v2 04/10] drm/i915: remove intel_crtc_cursor_set_obj()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Merge it into the plane update_plane() callback and make other
users use the update_plane() functions instead.

The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
and merge both paths into one.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 227 ++++++++++++++++-------------------
 1 file changed, 106 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f37e93..417b74d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8359,115 +8359,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-/*
- * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
- *
- * If the update succeeds, the reference of the old object (if any) will be
- * consumed.
- */
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 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
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-	}
-
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11800,7 +11691,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -11864,26 +11756,119 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	bool on;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 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
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
+
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
 
-		return 0;
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+	if (intel_crtc->cursor_bo == obj)
+		on = state->visible;
+	else
+		on = !obj;
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, on);
 	}
+
+	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	return 0;
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->base);
+	return ret;
 }
 
 static int
-- 
1.9.3

[PATCH v2 05/10] drm/i915: split intel_crtc_page_flip() into check() and commit()

[Gustavo Padovan]

From: Daniel Stone <daniels@collabora.com>

Start the work of splitting the intel_crtc_page_flip() for later use
by the atomic modesetting API.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 417b74d..f9ab8aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9543,23 +9543,11 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
 	spin_unlock(&dev->event_lock);
 }
 
-static int intel_crtc_page_flip(struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t page_flip_flags)
+static int intel_crtc_check_page_flip(struct drm_crtc *crtc,
+				      struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct intel_unpin_work *work;
-	struct intel_engine_cs *ring;
-	int ret;
-
-	//trigger software GT busyness calculation
-	gen8_flip_interrupt(dev);
 
 	/*
 	 * drm_mode_page_flip_ioctl() should already catch this, but double
@@ -9582,6 +9570,27 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
 		return -EINVAL;
 
+	return 0;
+}
+
+static int intel_crtc_commit_page_flip(struct drm_crtc *crtc,
+				       struct drm_framebuffer *fb,
+				       struct drm_pending_vblank_event *event,
+				       uint32_t page_flip_flags)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *old_fb = crtc->primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct intel_unpin_work *work;
+	struct intel_engine_cs *ring;
+	int ret;
+
+	/* trigger software GT busyness calculation */
+	gen8_flip_interrupt(dev);
+
 	if (i915_terminally_wedged(&dev_priv->gpu_error))
 		goto out_hang;
 
@@ -9725,6 +9734,20 @@ out_hang:
 	return ret;
 }
 
+static int intel_crtc_page_flip(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t page_flip_flags)
+{
+	int ret;
+
+	ret = intel_crtc_check_page_flip(crtc, fb);
+	if (ret)
+		return ret;
+
+	return intel_crtc_commit_page_flip(crtc, fb, event, page_flip_flags);
+}
+
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
-- 
1.9.3

[PATCH v2 06/10] drm: add helper to get crtc timings

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

We need to get hdisplay and vdisplay in a few places so create a
helper to make our job easier.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_crtc.c           | 20 +++++++++++++-------
 drivers/gpu/drm/i915/intel_display.c |  6 +++---
 include/drm/drm_crtc.h               |  2 ++
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b702106..7c0bf9f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2490,6 +2490,17 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 }
 EXPORT_SYMBOL(drm_mode_set_config_internal);
 
+void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+			    int *hdisplay, int *vdisplay)
+{
+	struct drm_display_mode adjusted = *mode;
+
+	drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
+	*hdisplay = adjusted.crtc_hdisplay;
+	*vdisplay = adjusted.crtc_vdisplay;
+}
+EXPORT_SYMBOL(drm_crtc_get_hv_timing);
+
 /**
  * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
  *     CRTC viewport
@@ -2510,13 +2521,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 	hdisplay = mode->hdisplay;
 	vdisplay = mode->vdisplay;
 
-	if (drm_mode_is_stereo(mode)) {
-		struct drm_display_mode adjusted = *mode;
-
-		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
-		hdisplay = adjusted.crtc_hdisplay;
-		vdisplay = adjusted.crtc_vdisplay;
-	}
+	if (drm_mode_is_stereo(mode))
+		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
 	if (crtc->invert_dimensions)
 		swap(hdisplay, vdisplay);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f9ab8aa..fd5251b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10056,9 +10056,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 * computation to clearly distinguish it from the adjusted mode, which
 	 * can be changed by the connectors in the below retry loop.
 	 */
-	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
-	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
-	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
+	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
+			       &pipe_config->pipe_src_w,
+			       &pipe_config->pipe_src_h);
 
 encoder_retry:
 	/* Ensure the port clock defaults are reset when retrying. */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c40070a..9b2f6b5 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -950,6 +950,8 @@ extern int drm_plane_init(struct drm_device *dev,
 extern void drm_plane_cleanup(struct drm_plane *plane);
 extern unsigned int drm_plane_index(struct drm_plane *plane);
 extern void drm_plane_force_disable(struct drm_plane *plane);
+extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+				   int *hdisplay, int *vdisplay);
 extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 				   int x, int y,
 				   const struct drm_display_mode *mode,
-- 
1.9.3

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

[PATCH v2 07/10] drm/i915: remove intel_pipe_set_base()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

After some refactor intel_primary_plane_setplane() does the same
as intel_pipe_set_base() so we can get rid of it and replace the calls
with intel_primary_plane_setplane().

v2: take Ville's comments:
	- get the right arguments for update_plane()
	- use drm_crtc_get_hv_timing()

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 89 ++++++++----------------------------
 1 file changed, 18 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fd5251b..0f4cb20 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2760,74 +2760,6 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
 	crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
 }
 
-static int
-intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
-		    struct drm_framebuffer *fb)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
-	int ret;
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
-	/* no fb bound */
-	if (!fb) {
-		DRM_ERROR("No FB bound\n");
-		return 0;
-	}
-
-	if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
-		DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n",
-			  plane_name(intel_crtc->plane),
-			  INTEL_INFO(dev)->num_pipes);
-		return -EINVAL;
-	}
-
-	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
-	if (ret == 0)
-		i915_gem_track_fb(old_obj, obj,
-				  INTEL_FRONTBUFFER_PRIMARY(pipe));
-	mutex_unlock(&dev->struct_mutex);
-	if (ret != 0) {
-		DRM_ERROR("pin & fence failed\n");
-		return ret;
-	}
-
-	intel_update_pipe_size(intel_crtc);
-
-	dev_priv->display.update_primary_plane(crtc, fb, x, y);
-
-	if (intel_crtc->active)
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
-
-	crtc->primary->fb = fb;
-	crtc->x = x;
-	crtc->y = y;
-
-	if (old_fb) {
-		if (intel_crtc->active && old_fb != fb)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	mutex_lock(&dev->struct_mutex);
-	intel_update_fbc(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	return 0;
-}
-
 static void intel_fdi_normal_train(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -9583,6 +9515,8 @@ static int intel_crtc_commit_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_plane *primary = crtc->primary;
+	struct intel_plane *intel_plane = to_intel_plane(primary);
 	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
@@ -9724,7 +9658,15 @@ free_work:
 	if (ret == -EIO) {
 out_hang:
 		intel_crtc_wait_for_pending_flips(crtc);
-		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
+		ret = primary->funcs->update_plane(primary, crtc, fb,
+						   intel_plane->crtc_x,
+						   intel_plane->crtc_y,
+						   intel_plane->crtc_h,
+						   intel_plane->crtc_w,
+						   intel_plane->src_x,
+						   intel_plane->src_y,
+						   intel_plane->src_h,
+						   intel_plane->src_w);
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
@@ -11261,11 +11203,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+		struct drm_plane *primary = set->crtc->primary;
+		int vdisplay, hdisplay;
 
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
-		ret = intel_pipe_set_base(set->crtc,
-					  set->x, set->y, set->fb);
+		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
+		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
+						   0, 0, hdisplay, vdisplay,
+						   set->x << 16, set->y << 16,
+						   hdisplay << 16, vdisplay << 16);
 
 		/*
 		 * We need to make sure the primary plane is re-enabled if it
-- 
1.9.3

[PATCH v2 08/10] drm/i915: create a prepare step for primary planes updates

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Take out the pin_fb code so commit phase can't fail anymore.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f4cb20..f7ca7ab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11437,20 +11437,16 @@ intel_check_primary_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_primary_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
+intel_prepare_primary_plane(struct drm_plane *plane,
+			    struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *old_fb = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_rect *src = &state->src;
 	int ret;
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11460,7 +11456,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		return -EBUSY;
 	}
 
-	if (plane->fb != fb) {
+	if (old_obj != obj) {
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 		if (ret == 0)
@@ -11473,6 +11469,25 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		}
 	}
 
+	return 0;
+}
+
+static void
+intel_commit_primary_plane(struct drm_plane *plane,
+			   struct intel_plane_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *old_fb = plane->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_rect *src = &state->src;
+
 	crtc->primary->fb = fb;
 	crtc->x = src->x1;
 	crtc->y = src->y1;
@@ -11544,8 +11559,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -11586,6 +11599,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	ret = intel_prepare_primary_plane(plane, &state);
+	if (ret)
+		return ret;
+
 	intel_commit_primary_plane(plane, &state);
 
 	return 0;
-- 
1.9.3

[PATCH v2 09/10] drm/i915: create a prepare phase for sprite plane updates

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

take out pin_fb code so the commit phase can't fail anymore.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 78044bb..e0c2fbc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1011,34 +1011,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_sprite_plane(struct drm_plane *plane,
-			  struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+			   struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *dst = &state->dst;
-	const struct drm_rect *clip = &state->clip;
-	bool primary_enabled;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(fb);
 	int ret;
 
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
 	if (old_obj != obj) {
 		mutex_lock(&dev->struct_mutex);
 
@@ -1057,6 +1041,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			return ret;
 	}
 
+	return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+			  struct intel_plane_state *state)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_crtc *crtc = state->crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *fb = state->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	int crtc_x, crtc_y;
+	unsigned int crtc_w, crtc_h;
+	uint32_t src_x, src_y, src_w, src_h;
+	struct drm_rect *dst = &state->dst;
+	const struct drm_rect *clip = &state->clip;
+	bool primary_enabled;
+
+	/*
+	 * If the sprite is completely covering the primary plane,
+	 * we can disable the primary and save power.
+	 */
+	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
 	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1117,8 +1131,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -1158,7 +1170,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	return intel_commit_sprite_plane(plane, &state);
+	ret = intel_prepare_sprite_plane(plane, &state);
+	if (ret)
+		return ret;
+
+	intel_commit_sprite_plane(plane, &state);
+	return 0;
 }
 
 static int
-- 
1.9.3

[PATCH v2 10/10] drm/i915: use intel_fb_obj() macros to assign gem objects

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Use the macros makes the code cleaner and it also checks for a NULL fb.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_sprite.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e0c2fbc..3778d19 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -851,8 +851,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
@@ -1054,9 +1053,8 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
-- 
1.9.3

Re: [PATCH v2 01/10] drm/i915: Merge of visible and !visible paths for primary planes

[Chris Wilson]

On Mon, Sep 22, 2014 at 07:23:08PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Fold intel_pipe_set_base() in the update primary plane path merging
> pieces of code that are common to both paths.
> 
> Basically the the pin/unpin procedures are the same for both paths
> and some checks can also be shared (some of the were moved to the
> check() stage)

Woah! Have you fixed the current regressions introduced by the earlier
patches?

xrandr <output> --rotate inverted is broken as the set-property calls
plane restore without the plane ever being configured, even though the
CRTC is active.

A sketchy and completely broken patch: http://patchwork.freedesktop.org/patch/33855/
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 01/10] drm/i915: Merge of visible and !visible paths for primary planes

[Ville Syrjälä]

On Mon, Sep 22, 2014 at 07:23:08PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Fold intel_pipe_set_base() in the update primary plane path merging
> pieces of code that are common to both paths.
> 
> Basically the the pin/unpin procedures are the same for both paths
> and some checks can also be shared (some of the were moved to the
> check() stage)
> 
> v2: take Ville's comments:
> 	- remove unnecessary plane check
> 	- move mutex lock to inside the conditional
> 	- make the pin fail message a debug one
> 	- add a fixme for the fastboot hack
> 	- call intel_frontbuffer_flip() after FBC update
> 
> v3: take more Ville's comments:
> 	- fold update code under if (intel_crtc->active), and do the
> 	visible/!visible split inside.
> 	- check ret inside the same conditional we assign it
> 
> v4: don't use intel_enable_primary_hw_plane(), the primary_enabled
> check inside will break page flips
> 
> v5: take more Ville's comments:
> 	- set primary_enabled to true and add BDW hack
> 	- unify if (old_fb) and if (old_fb != fb)
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 142 +++++++++++++++++++++--------------
>  1 file changed, 87 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 858011d..bef37dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11578,12 +11578,23 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	int ret;
>  
> -	return drm_plane_helper_check_update(plane, crtc, fb,
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    false, true, &state->visible);
> +	if (ret)
> +		return ret;
> +
> +	/* no fb bound */
> +	if (state->visible && !fb) {
> +		DRM_ERROR("No FB bound\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  static int
> @@ -11595,6 +11606,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum pipe pipe = intel_crtc->pipe;
> +	struct drm_framebuffer *old_fb = plane->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -11603,76 +11616,95 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
>  
> -	/*
> -	 * If clipping results in a non-visible primary plane, we'll disable
> -	 * the primary plane.  Note that this is a bit different than what
> -	 * happens if userspace explicitly disables the plane by passing fb=0
> -	 * because plane->fb still gets set and pinned.
> -	 */
> -	if (!state->visible) {
> +	if (intel_crtc_has_pending_flip(crtc)) {
> +		DRM_ERROR("pipe is still busy with an old pageflip\n");
> +		return -EBUSY;
> +	}
> +
> +	if (plane->fb != fb) {
>  		mutex_lock(&dev->struct_mutex);
> +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		if (ret == 0)
> +			i915_gem_track_fb(old_obj, obj,
> +					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +		if (ret != 0) {
> +			DRM_DEBUG_KMS("pin & fence failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	crtc->primary->fb = fb;
> +	crtc->x = src->x1;
> +	crtc->y = src->y1;
>  
> +	intel_plane->crtc_x = state->orig_dst.x1;
> +	intel_plane->crtc_y = state->orig_dst.y1;
> +	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> +	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> +	intel_plane->src_x = state->orig_src.x1;
> +	intel_plane->src_y = state->orig_src.y1;
> +	intel_plane->src_w = drm_rect_width(&state->orig_src);
> +	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	intel_plane->obj = obj;
> +
> +	if (intel_crtc->active) {
>  		/*
> -		 * Try to pin the new fb first so that we can bail out if we
> -		 * fail.
> +		 * FBC does not work on some platforms for rotated
> +		 * planes, so disable it when rotation is not 0 and
> +		 * update it when rotation is set back to 0.
> +		 *
> +		 * FIXME: This is redundant with the fbc update done in
> +		 * the primary plane enable function except that that
> +		 * one is done too late. We eventually need to unify
> +		 * this.
>  		 */
> -		if (plane->fb != fb) {
> -			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> -			if (ret) {
> -				mutex_unlock(&dev->struct_mutex);
> -				return ret;
> -			}
> +		if (intel_crtc->primary_enabled &&
> +		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +		    dev_priv->fbc.plane == intel_crtc->plane &&
> +		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> +			intel_disable_fbc(dev);
>  		}
>  
> -		i915_gem_track_fb(old_obj, obj,
> -				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -
> -		if (intel_crtc->primary_enabled)
> -			intel_disable_primary_hw_plane(plane, crtc);
> +		if (state->visible) {
> +			int was_enabled = intel_crtc->primary_enabled;

bool

>  
> +			/* FIXME: kill this fastboot hack */
> +			intel_update_pipe_size(intel_crtc);
>  
> -		if (plane->fb != fb)
> -			if (plane->fb)
> -				intel_unpin_fb_obj(old_obj);
> +			intel_crtc->primary_enabled = true;
>  
> -		mutex_unlock(&dev->struct_mutex);
> +			dev_priv->display.update_primary_plane(crtc, plane->fb,
> +					crtc->x, crtc->y);
>  
> -	} else {
> -		if (intel_crtc && intel_crtc->active &&
> -		    intel_crtc->primary_enabled) {
> +			if (IS_BROADWELL(dev) && was_enabled)

&& !was_enabled

and please bring over the comment from intel_enable_primary_hw_plane()
as well so that people don't have to wonder what this vblank wait is
doing here.

The rest of the patch looks good, so fix these and you get
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +				intel_wait_for_vblank(dev, intel_crtc->pipe);
> +		} else {
>  			/*
> -			 * FBC does not work on some platforms for rotated
> -			 * planes, so disable it when rotation is not 0 and
> -			 * update it when rotation is set back to 0.
> -			 *
> -			 * FIXME: This is redundant with the fbc update done in
> -			 * the primary plane enable function except that that
> -			 * one is done too late. We eventually need to unify
> -			 * this.
> +			 * If clipping results in a non-visible primary plane,
> +			 * we'll disable the primary plane.  Note that this is
> +			 * a bit different than what happens if userspace
> +			 * explicitly disables the plane by passing fb=0
> +			 * because plane->fb still gets set and pinned.
>  			 */
> -			if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -			    dev_priv->fbc.plane == intel_crtc->plane &&
> -			    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> -				intel_disable_fbc(dev);
> -			}
> +			intel_disable_primary_hw_plane(plane, crtc);
>  		}
> -		ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
> -		if (ret)
> -			return ret;
>  
> -		if (!intel_crtc->primary_enabled)
> -			intel_enable_primary_hw_plane(plane, crtc);
> +		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +
> +		mutex_lock(&dev->struct_mutex);
> +		intel_update_fbc(dev);
> +		mutex_unlock(&dev->struct_mutex);
>  	}
>  
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> -	intel_plane->obj = obj;
> +	if (old_fb && old_fb != fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +		mutex_lock(&dev->struct_mutex);
> +		intel_unpin_fb_obj(old_obj);
> +		mutex_unlock(&dev->struct_mutex);
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v2 02/10] drm/i915: remove leftover from pre-universal planes days

[Ville Syrjälä]

On Mon, Sep 22, 2014 at 07:23:09PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Now that universal planes are in place we don't need this plane unref on
> failures.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bef37dc..2ef1836 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8362,8 +8362,7 @@ static bool cursor_size_ok(struct drm_device *dev,
>  /*
>   * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
>   *
> - * Note that the object's reference will be consumed if the update fails.  If
> - * the update succeeds, the reference of the old object (if any) will be
> + * If the update succeeds, the reference of the old object (if any) will be
>   * consumed.

The rest of this comment should be removed as well. With universal
planes the fb holds the gem obj reference so we don't frob with
gem obj references at all here.

With that fixed this patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>   */
>  static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> @@ -8395,8 +8394,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	stride = roundup_pow_of_two(width) * 4;
>  	if (obj->base.size < stride * height) {
>  		DRM_DEBUG_KMS("buffer is too small\n");
> -		ret = -ENOMEM;
> -		goto fail;
> +		return -ENOMEM;
>  	}
>  
>  	/* we only need to pin inside GTT if cursor is non-phy */
> @@ -8485,8 +8483,6 @@ fail_unpin:
>  	i915_gem_object_unpin_from_display_plane(obj);
>  fail_locked:
>  	mutex_unlock(&dev->struct_mutex);
> -fail:
> -	drm_gem_object_unreference_unlocked(&obj->base);
>  	return ret;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out

[Ville Syrjälä]

On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Move checks inside intel_crtc_cursor_set_obj() to
> intel_check_cursor_plane(), we only use they there so move them out to
> make the merge of intel_crtc_cursor_set_obj() into
> intel_check_cursor_plane() easier.
> 
> This is another step toward the atomic modesetting support and unification
> of plane operations such pin/unpin of fb objects on i915.
> 
> v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the
> code
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ef1836..3f37e93 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> -	unsigned old_width, stride;
> +	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
> @@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  		goto finish;
>  	}
>  
> -	/* Check for which cursor types we support */
> -	if (!cursor_size_ok(dev, width, height)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> -		return -EINVAL;
> -	}
> -
> -	stride = roundup_pow_of_two(width) * 4;
> -	if (obj->base.size < stride * height) {
> -		DRM_DEBUG_KMS("buffer is too small\n");
> -		return -ENOMEM;
> -	}
> -
>  	/* we only need to pin inside GTT if cursor is non-phy */
>  	mutex_lock(&dev->struct_mutex);
>  	if (!INTEL_INFO(dev)->cursor_needs_physical) {
>  		unsigned alignment;
>  
> -		if (obj->tiling_mode) {
> -			DRM_DEBUG_KMS("cursor cannot be tiled\n");
> -			ret = -EINVAL;
> -			goto fail_locked;
> -		}
> -
>  		/*
>  		 * Global gtt pte registers are special registers which actually
>  		 * forward writes to a chunk of system memory. Which means that
> @@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	int crtc_w, crtc_h;
> +	unsigned stride;
> +	int ret;
>  
> -	return drm_plane_helper_check_update(plane, crtc, fb,
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    true, true, &state->visible);
> +	if (ret)
> +		return ret;
> +
> +
> +	/* if we want to turn off the cursor ignore width and height */
> +	if (!obj)
> +		return 0;
> +
> +	if (fb == crtc->cursor->fb)
> +		return 0;

Hmm. This check needs to be after the cursor/obj size checks. Otherwise
we wouldn't reject invalid sized cursors when the fb didn't change. I
suppose we could also just drop this check, but it can still save us from
doing the tiling check since that can't have changed if the fb hasn't
changed so maybe it's worth keeping. But either solution is fine by me.

> +
> +	/* Check for which cursor types we support */
> +	crtc_w = drm_rect_width(&state->orig_dst);
> +	crtc_h = drm_rect_height(&state->orig_dst);
> +	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> +		DRM_DEBUG("Cursor dimension not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	stride = roundup_pow_of_two(crtc_w) * 4;
> +	if (obj->base.size < stride * crtc_h) {
> +		DRM_DEBUG_KMS("buffer is too small\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* we only need to pin inside GTT if cursor is non-phy */
> +	mutex_lock(&dev->struct_mutex);
> +	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
> +		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;

'return 0' seems better here.

With these issues fixed:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static int
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out

[Gustavo Padovan]

2014-09-23 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Move checks inside intel_crtc_cursor_set_obj() to
> > intel_check_cursor_plane(), we only use they there so move them out to
> > make the merge of intel_crtc_cursor_set_obj() into
> > intel_check_cursor_plane() easier.
> > 
> > This is another step toward the atomic modesetting support and unification
> > of plane operations such pin/unpin of fb objects on i915.
> > 
> > v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the
> > code
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------
> >  1 file changed, 41 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2ef1836..3f37e93 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> > -	unsigned old_width, stride;
> > +	unsigned old_width;
> >  	uint32_t addr;
> >  	int ret;
> >  
> > @@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> >  		goto finish;
> >  	}
> >  
> > -	/* Check for which cursor types we support */
> > -	if (!cursor_size_ok(dev, width, height)) {
> > -		DRM_DEBUG("Cursor dimension not supported\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	stride = roundup_pow_of_two(width) * 4;
> > -	if (obj->base.size < stride * height) {
> > -		DRM_DEBUG_KMS("buffer is too small\n");
> > -		return -ENOMEM;
> > -	}
> > -
> >  	/* we only need to pin inside GTT if cursor is non-phy */
> >  	mutex_lock(&dev->struct_mutex);
> >  	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> >  		unsigned alignment;
> >  
> > -		if (obj->tiling_mode) {
> > -			DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > -			ret = -EINVAL;
> > -			goto fail_locked;
> > -		}
> > -
> >  		/*
> >  		 * Global gtt pte registers are special registers which actually
> >  		 * forward writes to a chunk of system memory. Which means that
> > @@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  			 struct intel_plane_state *state)
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> > +	struct drm_device *dev = crtc->dev;
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_rect *dest = &state->dst;
> >  	struct drm_rect *src = &state->src;
> >  	const struct drm_rect *clip = &state->clip;
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +	int crtc_w, crtc_h;
> > +	unsigned stride;
> > +	int ret;
> >  
> > -	return drm_plane_helper_check_update(plane, crtc, fb,
> > +	ret = drm_plane_helper_check_update(plane, crtc, fb,
> >  					    src, dest, clip,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    true, true, &state->visible);
> > +	if (ret)
> > +		return ret;
> > +
> > +
> > +	/* if we want to turn off the cursor ignore width and height */
> > +	if (!obj)
> > +		return 0;
> > +
> > +	if (fb == crtc->cursor->fb)
> > +		return 0;
> 
> Hmm. This check needs to be after the cursor/obj size checks. Otherwise
> we wouldn't reject invalid sized cursors when the fb didn't change. I
> suppose we could also just drop this check, but it can still save us from
> doing the tiling check since that can't have changed if the fb hasn't
> changed so maybe it's worth keeping. But either solution is fine by me.

Only if this was a bug before this patch, if you look at the original
intel_commit_cursor_plane() we do two things when the fbs are the same:
               
	intel_crtc_update_cursor();
	intel_frontbuffer_flip();

No checks are performed, that is why I put the fbs check before any other
check to make the code as similar as possible to what it was before.

> > +
> > +	/* Check for which cursor types we support */
> > +	crtc_w = drm_rect_width(&state->orig_dst);
> > +	crtc_h = drm_rect_height(&state->orig_dst);
> > +	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> > +		DRM_DEBUG("Cursor dimension not supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	stride = roundup_pow_of_two(crtc_w) * 4;
> > +	if (obj->base.size < stride * crtc_h) {
> > +		DRM_DEBUG_KMS("buffer is too small\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* we only need to pin inside GTT if cursor is non-phy */
> > +	mutex_lock(&dev->struct_mutex);
> > +	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
> > +		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > +		ret = -EINVAL;
> > +	}
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	return ret;
> 
> 'return 0' seems better here.

ret can be -EINVAL, the code inside the mutex 3 lines above can set ret.

	Gustavo

Re: [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out

[Ville Syrjälä]

On Tue, Sep 23, 2014 at 12:41:56PM -0300, Gustavo Padovan wrote:
> 2014-09-23 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Move checks inside intel_crtc_cursor_set_obj() to
> > > intel_check_cursor_plane(), we only use they there so move them out to
> > > make the merge of intel_crtc_cursor_set_obj() into
> > > intel_check_cursor_plane() easier.
> > > 
> > > This is another step toward the atomic modesetting support and unification
> > > of plane operations such pin/unpin of fb objects on i915.
> > > 
> > > v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the
> > > code
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------
> > >  1 file changed, 41 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2ef1836..3f37e93 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	enum pipe pipe = intel_crtc->pipe;
> > > -	unsigned old_width, stride;
> > > +	unsigned old_width;
> > >  	uint32_t addr;
> > >  	int ret;
> > >  
> > > @@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> > >  		goto finish;
> > >  	}
> > >  
> > > -	/* Check for which cursor types we support */
> > > -	if (!cursor_size_ok(dev, width, height)) {
> > > -		DRM_DEBUG("Cursor dimension not supported\n");
> > > -		return -EINVAL;
> > > -	}
> > > -
> > > -	stride = roundup_pow_of_two(width) * 4;
> > > -	if (obj->base.size < stride * height) {
> > > -		DRM_DEBUG_KMS("buffer is too small\n");
> > > -		return -ENOMEM;
> > > -	}
> > > -
> > >  	/* we only need to pin inside GTT if cursor is non-phy */
> > >  	mutex_lock(&dev->struct_mutex);
> > >  	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > >  		unsigned alignment;
> > >  
> > > -		if (obj->tiling_mode) {
> > > -			DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > > -			ret = -EINVAL;
> > > -			goto fail_locked;
> > > -		}
> > > -
> > >  		/*
> > >  		 * Global gtt pte registers are special registers which actually
> > >  		 * forward writes to a chunk of system memory. Which means that
> > > @@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >  			 struct intel_plane_state *state)
> > >  {
> > >  	struct drm_crtc *crtc = state->crtc;
> > > +	struct drm_device *dev = crtc->dev;
> > >  	struct drm_framebuffer *fb = state->fb;
> > >  	struct drm_rect *dest = &state->dst;
> > >  	struct drm_rect *src = &state->src;
> > >  	const struct drm_rect *clip = &state->clip;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	int crtc_w, crtc_h;
> > > +	unsigned stride;
> > > +	int ret;
> > >  
> > > -	return drm_plane_helper_check_update(plane, crtc, fb,
> > > +	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > >  					    src, dest, clip,
> > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > >  					    true, true, &state->visible);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +
> > > +	/* if we want to turn off the cursor ignore width and height */
> > > +	if (!obj)
> > > +		return 0;
> > > +
> > > +	if (fb == crtc->cursor->fb)
> > > +		return 0;
> > 
> > Hmm. This check needs to be after the cursor/obj size checks. Otherwise
> > we wouldn't reject invalid sized cursors when the fb didn't change. I
> > suppose we could also just drop this check, but it can still save us from
> > doing the tiling check since that can't have changed if the fb hasn't
> > changed so maybe it's worth keeping. But either solution is fine by me.
> 
> Only if this was a bug before this patch, if you look at the original
> intel_commit_cursor_plane() we do two things when the fbs are the same:
>                
> 	intel_crtc_update_cursor();
> 	intel_frontbuffer_flip();
> 
> No checks are performed, that is why I put the fbs check before any other
> check to make the code as similar as possible to what it was before.

I guess we already had the bug then. Not entirely unsurprising given the
recent history of this code. Probably better to have the fix as a separate
patch then to avoid mixing restructuring and functional changes in the
same patch.

> 
> > > +
> > > +	/* Check for which cursor types we support */
> > > +	crtc_w = drm_rect_width(&state->orig_dst);
> > > +	crtc_h = drm_rect_height(&state->orig_dst);
> > > +	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> > > +		DRM_DEBUG("Cursor dimension not supported\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	stride = roundup_pow_of_two(crtc_w) * 4;
> > > +	if (obj->base.size < stride * crtc_h) {
> > > +		DRM_DEBUG_KMS("buffer is too small\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	/* we only need to pin inside GTT if cursor is non-phy */
> > > +	mutex_lock(&dev->struct_mutex);
> > > +	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
> > > +		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > > +		ret = -EINVAL;
> > > +	}
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > > +	return ret;
> > 
> > 'return 0' seems better here.
> 
> ret can be -EINVAL, the code inside the mutex 3 lines above can set ret.

Oh, somehow I missed that one. You can leave the patch as is then.

> 
> 	Gustavo

-- 
Ville Syrjälä
Intel OTC