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

[PATCH v3 01/11] 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)

v6: take more Ville's comments:
	- make was_primary bool and fix its check
	- add the BDW vblank wait comment

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

fixup! drm/i915: Merge of visible and !visible paths for primary planes
---
 drivers/gpu/drm/i915/intel_display.c | 147 ++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8488a8..966b9af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11676,12 +11676,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
@@ -11693,6 +11704,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);
@@ -11701,76 +11714,100 @@ 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) {
+			bool 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) {
 			/*
-			 * 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.
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
 			 */
-			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);
-			}
+			if (IS_BROADWELL(dev) && !was_enabled)
+				intel_wait_for_vblank(dev, intel_crtc->pipe);
+		} else {
+			/*
+			 * 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.
+			 */
+			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

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

[PATCH v3 02/11] 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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 966b9af..b1c2dbf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8457,13 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-/*
- * 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
- * consumed.
- */
 static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     struct drm_i915_gem_object *obj,
 				     uint32_t width, uint32_t height)
@@ -8493,8 +8486,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 */
@@ -8583,8 +8575,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

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

[PATCH v3 03/11] drm/i915: Fix not checking cursor and object sizes

[Gustavo Padovan]

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

Even if the fb is the same we should still check if the sizes are
valid to be set.

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 b1c2dbf..20be2ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8465,7 +8465,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;
 
@@ -8477,29 +8477,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
@@ -11923,16 +11905,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 v3 04/11] drm/i915: move check of intel_crtc_cursor_set_obj() out

[Gustavo Padovan]

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

Move check inside intel_crtc_cursor_set_obj() to
intel_check_cursor_plane(), we only use it 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
v3: take Ville's comment: kept only the restructuring changes, the rest of
the code was moved to a separated patch since it is a bug fix (we weren't
checking sizes when the fb was the same)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 20be2ed..f91a5b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	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);
@@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		return -ENOMEM;
 	}
 
+	if (fb == crtc->cursor->fb)
+		return 0;
+
 	/* 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) {
-- 
1.9.3

[PATCH v3 05/11] 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 | 221 +++++++++++++++++------------------
 1 file changed, 106 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f91a5b0..a583abd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-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)
 {
@@ -11897,7 +11794,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
@@ -11961,26 +11859,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;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	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 v3 06/11] 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 a583abd..3cb092c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9641,23 +9641,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
@@ -9680,6 +9668,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;
 
@@ -9823,6 +9832,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 v3 07/11] 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 3cb092c..dfe9ad5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10154,9 +10154,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

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

[PATCH v3 08/11] 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 dfe9ad5..9dd4952 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11588,20 +11588,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);
@@ -11611,7 +11607,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)
@@ -11624,6 +11620,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;
@@ -11700,8 +11715,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -11742,6 +11755,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 v3 09/11] 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 750b634..8e5445b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1189,34 +1189,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(plane->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);
 
@@ -1235,6 +1219,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);
@@ -1295,8 +1309,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -1336,7 +1348,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 v3 10/11] 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 8e5445b..5cb7321 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1029,8 +1029,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;
@@ -1232,9 +1231,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

[PATCH v3 11/11] 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 9dd4952..f7c2e5f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2857,74 +2857,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;
@@ -9681,6 +9613,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;
@@ -9822,7 +9756,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);
@@ -11359,11 +11301,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

Re: [PATCH v3 04/11] drm/i915: move check of intel_crtc_cursor_set_obj() out

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Move check inside intel_crtc_cursor_set_obj() to
> intel_check_cursor_plane(), we only use it 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
> v3: take Ville's comment: kept only the restructuring changes, the rest of
> the code was moved to a separated patch since it is a bug fix (we weren't
> checking sizes when the fb was the same)
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Looks like the commit messages for patches 2 and 3 got somehow swapped
around. With that fixed patches 2 and 3 get:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 20be2ed..f91a5b0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	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);
> @@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> +	if (fb == crtc->cursor->fb)
> +		return 0;
> +
>  	/* 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) {
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> 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 | 221 +++++++++++++++++------------------
>  1 file changed, 106 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f91a5b0..a583abd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
>  	return true;
>  }
>  
> -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)
>  {
> @@ -11897,7 +11794,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
> @@ -11961,26 +11859,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;
> +	}
>  
> -		intel_frontbuffer_flip(crtc->dev,
> -				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> +finish:
> +	if (intel_crtc->cursor_bo) {
> +		if (!INTEL_INFO(dev)->cursor_needs_physical)
> +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> +	}
>  
> -		return 0;
> +	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> +			  INTEL_FRONTBUFFER_CURSOR(pipe));
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	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;

That looks fishy. Why do we need to care if the bo changed here, and why
would we turn on the cursor when there's no bo?

The cursor is either visible or it isn't, and that's all that
intel_crtc_update_cursor() should care about.

> +
> +	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));

This should probably be done only when crtc->active==true. I see we're
not doing it that way currently, so I guess we could have a separate
patch to change that.

> +
> +	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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 04/11] drm/i915: move check of intel_crtc_cursor_set_obj() out

[Ville Syrjälä]

On Tue, Oct 07, 2014 at 05:47:52PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 24, 2014 at 02:20:25PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Move check inside intel_crtc_cursor_set_obj() to
> > intel_check_cursor_plane(), we only use it 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
> > v3: take Ville's comment: kept only the restructuring changes, the rest of
> > the code was moved to a separated patch since it is a bug fix (we weren't
> > checking sizes when the fb was the same)
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Looks like the commit messages for patches 2 and 3 got somehow swapped
> around. With that fixed patches 2 and 3 get:

And make that patches 3 and 4 and we're actaully talking about the
correct two patches.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 20be2ed..f91a5b0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  	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);
> > @@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  		return -ENOMEM;
> >  	}
> >  
> > +	if (fb == crtc->cursor->fb)
> > +		return 0;
> > +
> >  	/* 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) {
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 07/11] drm: add helper to get crtc timings

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:28PM -0300, Gustavo Padovan wrote:
> 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;

Hmm. Now that I read drm_mode_set_crtcinfo() it would seem we'll do
the wrong thing when dealing with doublescan or vscan>1. That problem
is already true without your patch, but we should do something to fix
it anyway.

Maybe the best option would be to yank the stereo doubling logic from
drm_mode_set_crtcinfo() into a separate function and call it both here
and in drm_mode_set_crtcinfo(). That would avoid this problem without
duplicating the specifics of stereo doubling in multiple places.

> +}
> +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;

With the suggested doublescan/vscan fix you could kill these
assignments too and just call drm_crtc_get_hv_timing() unconditionally.

>  
> -	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 3cb092c..dfe9ad5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10154,9 +10154,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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 06/11] drm/i915: split intel_crtc_page_flip() into check() and commit()

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:27PM -0300, Gustavo Padovan wrote:
> From: Daniel Stone <daniels@collabora.com>
> 
> Start the work of splitting the intel_crtc_page_flip() for later use
> by the atomic modesetting API.

At this time this doesn't really do anything so I don't see much point
in applying it, for now at least.

> 
> 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 a583abd..3cb092c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9641,23 +9641,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
> @@ -9680,6 +9668,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;
>  
> @@ -9823,6 +9832,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 09/11] drm/i915: create a prepare phase for sprite plane updates

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:30PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> take out pin_fb code so the commit phase can't fail anymore.

Yeah making commit() void is a good step. For patches 8 and 9:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 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 750b634..8e5445b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1189,34 +1189,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(plane->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);
>  
> @@ -1235,6 +1219,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);
> @@ -1295,8 +1309,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  		intel_unpin_fb_obj(old_obj);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> -
> -	return 0;
>  }
>  
>  static int
> @@ -1336,7 +1348,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v3 10/11] drm/i915: use intel_fb_obj() macros to assign gem objects

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:31PM -0300, Gustavo Padovan wrote:
> 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>

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

> ---
>  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 8e5445b..5cb7321 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1029,8 +1029,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;
> @@ -1232,9 +1231,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 11/11] drm/i915: remove intel_pipe_set_base()

[Ville Syrjälä]

On Wed, Sep 24, 2014 at 02:20:32PM -0300, Gustavo Padovan wrote:
> 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 9dd4952..f7c2e5f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2857,74 +2857,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;
> @@ -9681,6 +9613,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;
> @@ -9822,7 +9756,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);
> @@ -11359,11 +11301,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);
>  

These two look like they should do the right thing. But we still have
the mode_change==true path to worry about. If we don't call
update_plane() there we migth never populate intel_plane->crtc_x & co.

So I think you'll just need to replace the small piece of pin/unpin code
in __intel_set_mode() with a call to update_plane(). Since the crtc is
not active at that point it will skip the actual plane register
programming and intel_enable_primary_hw_plane() will later do all that.

>  		/*
>  		 * We need to make sure the primary plane is re-enabled if it
> -- 
> 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 v3 04/11] drm/i915: move check of intel_crtc_cursor_set_obj() out

[Daniel Vetter]

On Tue, Oct 07, 2014 at 06:01:10PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 07, 2014 at 05:47:52PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 24, 2014 at 02:20:25PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > >
> > > Move check inside intel_crtc_cursor_set_obj() to
> > > intel_check_cursor_plane(), we only use it 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
> > > v3: take Ville's comment: kept only the restructuring changes, the rest of
> > > the code was moved to a separated patch since it is a bug fix (we weren't
> > > checking sizes when the fb was the same)
> > >
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > Looks like the commit messages for patches 2 and 3 got somehow swapped
> > around. With that fixed patches 2 and 3 get:
>
> And make that patches 3 and 4 and we're actaully talking about the
> correct two patches.

Ok, I think I've fixed it up.
>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged up to this patch to dinq.
-Daniel

> >
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 20be2ed..f91a5b0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >   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);
> > > @@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >   return -ENOMEM;
> > >   }
> > >
> > > + if (fb == crtc->cursor->fb)
> > > + return 0;
> > > +
> > >   /* 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) {
> > > --
> > > 1.9.3
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()

[Gustavo Padovan]

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

> On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> > 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 | 221 +++++++++++++++++------------------
> >  1 file changed, 106 insertions(+), 115 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f91a5b0..a583abd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> >  	return true;
> >  }
> >  
> > -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)
> >  {
> > @@ -11897,7 +11794,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
> > @@ -11961,26 +11859,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;
> > +	}
> >  
> > -		intel_frontbuffer_flip(crtc->dev,
> > -				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> > +finish:
> > +	if (intel_crtc->cursor_bo) {
> > +		if (!INTEL_INFO(dev)->cursor_needs_physical)
> > +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > +	}
> >  
> > -		return 0;
> > +	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > +			  INTEL_FRONTBUFFER_CURSOR(pipe));
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	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;
> 
> That looks fishy. Why do we need to care if the bo changed here, and why
> would we turn on the cursor when there's no bo?

Yes, this should actually have been on = !!obj. But all I was doing here is
reordering the code, so this is how it was before. The flow is the same.

> 
> The cursor is either visible or it isn't, and that's all that
> intel_crtc_update_cursor() should care about.

Right, I'll remove the if-else and call intel_crtc_update_cursor() with
state->visible as parameter.

> 
> > +
> > +	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));
> 
> This should probably be done only when crtc->active==true. I see we're
> not doing it that way currently, so I guess we could have a separate
> patch to change that.

Okay, I have created a separated patch for this.

	Gustavo

Re: [PATCH v3 11/11] drm/i915: remove intel_pipe_set_base()

[Gustavo Padovan]

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

> On Wed, Sep 24, 2014 at 02:20:32PM -0300, Gustavo Padovan wrote:
> > 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 9dd4952..f7c2e5f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2857,74 +2857,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;
> > @@ -9681,6 +9613,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;
> > @@ -9822,7 +9756,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);
> > @@ -11359,11 +11301,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);
> >  
> 
> These two look like they should do the right thing. But we still have
> the mode_change==true path to worry about. If we don't call
> update_plane() there we migth never populate intel_plane->crtc_x & co.
> 
> So I think you'll just need to replace the small piece of pin/unpin code
> in __intel_set_mode() with a call to update_plane(). Since the crtc is
> not active at that point it will skip the actual plane register
> programming and intel_enable_primary_hw_plane() will later do all that.

Yes, this is a pending patch that I have here. It does exactly what you are
saying but it breaks everything. It needs more work I think, the simple
replacement by an update_plane() doesn't work.

	Gustavo

Re: [PATCH v3 11/11] drm/i915: remove intel_pipe_set_base()

[Ville Syrjälä]

On Fri, Oct 24, 2014 at 02:59:44PM +0100, Gustavo Padovan wrote:
> 2014-10-07 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Wed, Sep 24, 2014 at 02:20:32PM -0300, Gustavo Padovan wrote:
> > > 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 9dd4952..f7c2e5f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2857,74 +2857,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;
> > > @@ -9681,6 +9613,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;
> > > @@ -9822,7 +9756,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);
> > > @@ -11359,11 +11301,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);
> > >  
> > 
> > These two look like they should do the right thing. But we still have
> > the mode_change==true path to worry about. If we don't call
> > update_plane() there we migth never populate intel_plane->crtc_x & co.
> > 
> > So I think you'll just need to replace the small piece of pin/unpin code
> > in __intel_set_mode() with a call to update_plane(). Since the crtc is
> > not active at that point it will skip the actual plane register
> > programming and intel_enable_primary_hw_plane() will later do all that.
> 
> Yes, this is a pending patch that I have here. It does exactly what you are
> saying but it breaks everything. It needs more work I think, the simple
> replacement by an update_plane() doesn't work.

Hmm. I would have expected it to work. I think I need to change my
default back to pessimism so I'll avoid being disappointed :)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()

[Ville Syrjälä]

On Fri, Oct 24, 2014 at 02:23:35PM +0100, Gustavo Padovan wrote:
> 2014-10-07 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> > > 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 | 221 +++++++++++++++++------------------
> > >  1 file changed, 106 insertions(+), 115 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f91a5b0..a583abd 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> > >  	return true;
> > >  }
> > >  
> > > -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)
> > >  {
> > > @@ -11897,7 +11794,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
> > > @@ -11961,26 +11859,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;
> > > +	}
> > >  
> > > -		intel_frontbuffer_flip(crtc->dev,
> > > -				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> > > +finish:
> > > +	if (intel_crtc->cursor_bo) {
> > > +		if (!INTEL_INFO(dev)->cursor_needs_physical)
> > > +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > > +	}
> > >  
> > > -		return 0;
> > > +	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > > +			  INTEL_FRONTBUFFER_CURSOR(pipe));
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > > +	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;
> > 
> > That looks fishy. Why do we need to care if the bo changed here, and why
> > would we turn on the cursor when there's no bo?
> 
> Yes, this should actually have been on = !!obj. But all I was doing here is
> reordering the code, so this is how it was before. The flow is the same.
> 
> > 
> > The cursor is either visible or it isn't, and that's all that
> > intel_crtc_update_cursor() should care about.
> 
> Right, I'll remove the if-else and call intel_crtc_update_cursor() with
> state->visible as parameter.

I just realized that we don't seem to actually compute state->visible
correctly when there's no fb. So that definitely needs to be done if we
want to share the .update_plane() as .disable_plane().

> 
> > 
> > > +
> > > +	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));
> > 
> > This should probably be done only when crtc->active==true. I see we're
> > not doing it that way currently, so I guess we could have a separate
> > patch to change that.
> 
> Okay, I have created a separated patch for this.
> 
> 	Gustavo

-- 
Ville Syrjälä
Intel OTC