[PATCH 0/5] drm/i915: Interruptible framebuffer pinning.

[PATCH 0/5] drm/i915: Interruptible framebuffer pinning.

[Maarten Lankhorst]

With the changes to drm/core to preserve framebuffers there is no more barrier for allowing
pin to fail with -ERESTARTSYS. Modesetting while the gpu died should still be allowed,
so any -EIO during pin stage will be eaten.

Some display related waits that were previously uninterruptible can now be interrupted,
and will be done before committing the new state. This will allow abort to work correctly.

This patch series requires topic/drm-misc to be applied, but can also be applied to that branch directly.

Maarten Lankhorst (5):
  drm/i915: Make plane fb tracking work correctly, v2.
  drm/i915: Make prepare_plane_fb fully interruptible.
  drm/i915: Make wait_for_flips interruptible.
  drm/i915: Change locking for struct_mutex.
  drm/i915: Wait for object idle without locks in atomic_commit.

 drivers/gpu/drm/i915/i915_drv.h           |   2 -
 drivers/gpu/drm/i915/i915_gem.c           |   6 -
 drivers/gpu/drm/i915/intel_atomic.c       |   2 -
 drivers/gpu/drm/i915/intel_atomic_plane.c |   2 +
 drivers/gpu/drm/i915/intel_display.c      | 252 +++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h          |  10 +-
 drivers/gpu/drm/i915/intel_fbdev.c        |   2 +-
 drivers/gpu/drm/i915/intel_overlay.c      |   6 +-
 8 files changed, 169 insertions(+), 113 deletions(-)

-- 
2.1.0

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

[PATCH 1/5] drm/i915: Make plane fb tracking work correctly, v2.

[Maarten Lankhorst]

atomic->disabled_planes is a hack that had to exist because
prepare_fb was only called when a new fb was set. This messed
up fb tracking in some circumstances like aborts from
interruptible waits. As a result interruptible waiting in
prepare_plane_fb was forbidden, but other errors could still
cause frontbuffer tracking to be messed up.

Now that prepare_fb is always called, this hack is no longer
required and prepare_fb may fail without consequences.

Changes since v1:
- Clean up a few fb tracking warnings by changing plane->fb to
  plane->state->fb.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fc0086748b71..ac97af69be62 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4752,17 +4752,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
-	struct drm_plane *p;
-
-	/* Track fb's for any planes being disabled */
-	drm_for_each_plane_mask(p, dev, atomic->disabled_planes) {
-		struct intel_plane *plane = to_intel_plane(p);
-
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->base.fb), NULL,
-				  plane->frontbuffer_bit);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (atomic->wait_for_flips)
 		intel_crtc_wait_for_pending_flips(&crtc->base);
@@ -11561,14 +11550,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			return ret;
 	}
 
-	/*
-	 * Disabling a plane is always okay; we just need to update
-	 * fb tracking in a special way since cleanup_fb() won't
-	 * get called by the plane helpers.
-	 */
-	if (old_plane_state->base.fb && !fb)
-		intel_crtc->atomic.disabled_planes |= 1 << i;
-
 	was_visible = old_plane_state->visible;
 	visible = to_intel_plane_state(plane_state)->visible;
 
@@ -13318,15 +13299,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct drm_framebuffer *fb = new_state->fb;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
 	int ret = 0;
 
-	if (!obj)
+	if (!obj && !old_obj)
 		return 0;
 
 	mutex_lock(&dev->struct_mutex);
 
-	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+	if (!obj) {
+		ret = 0;
+	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
 	    INTEL_INFO(dev)->cursor_needs_physical) {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
 		ret = i915_gem_object_attach_phys(obj, align);
@@ -13356,17 +13339,23 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_gem_object *obj = intel_fb_obj(old_state->fb);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
+	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
 
-	if (!obj)
+	if (!obj && !old_obj)
 		return;
 
-	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev)->cursor_needs_physical) {
-		mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev->struct_mutex);
+	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
+	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state);
-		mutex_unlock(&dev->struct_mutex);
-	}
+
+	/* prepare_fb aborted? */
+	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
+	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
+		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a3bed85ab564..a2f0b981e26c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -505,7 +505,6 @@ struct intel_crtc_atomic_commit {
 	bool disable_cxsr;
 	bool pre_disable_primary;
 	bool update_wm_pre, update_wm_post;
-	unsigned disabled_planes;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-- 
2.1.0

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

[PATCH 2/5] drm/i915: Make prepare_plane_fb fully interruptible.

[Maarten Lankhorst]

Now that we agreed on not preserving framebuffers pinning is finally
allowed to fail because of signals. Use this to make pinning
and acquire the mutex in an interruptible way too.

Unpinning is still uninterruptible, because it happens as a cleanup
of old state, or undoing pins after one of the pins failed.

The intel_pin_and_fence_fb_obj in page_flip will also wait interruptibly,
and can be aborted now.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac97af69be62..25e1eac260fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2349,11 +2349,10 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 */
 	intel_runtime_pm_get(dev_priv);
 
-	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
 						   pipelined_request, &view);
 	if (ret)
-		goto err_interruptible;
+		goto err_pm;
 
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
@@ -2377,14 +2376,12 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 
 	i915_gem_object_pin_fence(obj);
 
-	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj, &view);
-err_interruptible:
-	dev_priv->mm.interruptible = true;
+err_pm:
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
@@ -13305,7 +13302,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return 0;
 
-	mutex_lock(&dev->struct_mutex);
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
 
 	if (!obj) {
 		ret = 0;
-- 
2.1.0

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

[PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Maarten Lankhorst]

Move it from intel_crtc_atomic_commit to prepare_plane_fb.
Waiting is done before committing, otherwise it's too late
to undo the changes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index f1975f267710..25a891aa3824 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just do the
 				 * minimum required validation.
 				 */
-				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-					intel_crtc->atomic.wait_for_flips = true;
 				crtc_state->base.planes_changed = true;
 			}
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 25e1eac260fd..cd651ff6c15b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
 	drm_modeset_unlock_all(dev);
 }
 
-static void
-intel_finish_fb(struct drm_framebuffer *old_fb)
-{
-	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
-	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
-	bool was_interruptible = dev_priv->mm.interruptible;
-	int ret;
-
-	/* Big Hammer, we also need to ensure that any pending
-	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
-	 * current scanout is retired before unpinning the old
-	 * framebuffer. Note that we rely on userspace rendering
-	 * into the buffer attached to the pipe they are waiting
-	 * on. If not, userspace generates a GPU hang with IPEHR
-	 * point to the MI_WAIT_FOR_EVENT.
-	 *
-	 * This should only fail upon a hung GPU, in which case we
-	 * can safely continue.
-	 */
-	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_wait_rendering(obj, true);
-	dev_priv->mm.interruptible = was_interruptible;
-
-	WARN_ON(ret);
-}
-
 static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc *intel_crtc)
 				 work->pending_flip_obj);
 }
 
-void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
+static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	long ret;
 
 	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
-	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
-				       !intel_crtc_has_pending_flip(crtc),
-				       60*HZ) == 0)) {
+
+	ret = wait_event_interruptible_timeout(
+					dev_priv->pending_flip_queue,
+					!intel_crtc_has_pending_flip(crtc),
+					60*HZ);
+
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		spin_lock_irq(&dev->event_lock);
@@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 		spin_unlock_irq(&dev->event_lock);
 	}
 
-	if (crtc->primary->fb) {
-		mutex_lock(&dev->struct_mutex);
-		intel_finish_fb(crtc->primary->fb);
-		mutex_unlock(&dev->struct_mutex);
-	}
+	return 0;
 }
 
 /* Program iCLKIP clock to the desired frequency */
@@ -4750,9 +4728,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 
-	if (atomic->wait_for_flips)
-		intel_crtc_wait_for_pending_flips(&crtc->base);
-
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
 
@@ -11596,7 +11571,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.wait_for_flips = true;
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
@@ -13015,6 +12989,30 @@ static int intel_atomic_check(struct drm_device *dev,
 	return drm_atomic_helper_check_planes(state->dev, state);
 }
 
+static int intel_atomic_prepare_commit(struct drm_device *dev,
+				       struct drm_atomic_state *state,
+				       bool async)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i, ret;
+
+	if (async) {
+		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
+		return -EINVAL;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		ret = intel_crtc_wait_for_pending_flips(crtc);
+		if (ret)
+			return ret;
+	}
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+
+	return ret;
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13042,12 +13040,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	int i;
 	bool any_ms = false;
 
-	if (async) {
-		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
-		return -EINVAL;
-	}
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
+	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret)
 		return ret;
 
@@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	if (old_obj) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
+
+		/* Big Hammer, we also need to ensure that any pending
+		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
+		 * current scanout is retired before unpinning the old
+		 * framebuffer. Note that we rely on userspace rendering
+		 * into the buffer attached to the pipe they are waiting
+		 * on. If not, userspace generates a GPU hang with IPEHR
+		 * point to the MI_WAIT_FOR_EVENT.
+		 *
+		 * This should only fail upon a hung GPU, in which case we
+		 * can safely continue.
+		 */
+		if (needs_modeset(crtc_state))
+			ret = i915_gem_object_wait_rendering(old_obj, true);
+
+		/* Swallow -EIO errors to allow updates during hw lockup. */
+		if (ret && ret != -EIO)
+			goto out;
+	}
+
 	if (!obj) {
 		ret = 0;
 	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
@@ -13321,6 +13337,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret == 0)
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
 
+out:
 	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a2f0b981e26c..cfdb0f2714cd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,7 +499,6 @@ struct skl_pipe_wm {
  */
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
-	bool wait_for_flips;
 	bool disable_fbc;
 	bool disable_ips;
 	bool disable_cxsr;
@@ -1158,7 +1157,6 @@ enum intel_display_power_domain
 intel_display_port_power_domain(struct intel_encoder *intel_encoder);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
-void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
-- 
2.1.0

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

[PATCH 4/5] drm/i915: Change locking for struct_mutex.

[Maarten Lankhorst]

Only acquire the struct_mutex once, and interruptibly.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cd651ff6c15b..2f046134cc9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13008,8 +13008,13 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 			return ret;
 	}
 
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 
+	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
 
@@ -13108,7 +13113,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 	/* FIXME: add subpixel order */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (any_ms)
 		intel_modeset_check_state(dev, state);
@@ -13295,10 +13303,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return 0;
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
-
 	if (old_obj) {
 		struct drm_crtc_state *crtc_state =
 			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
@@ -13319,7 +13323,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 		/* Swallow -EIO errors to allow updates during hw lockup. */
 		if (ret && ret != -EIO)
-			goto out;
+			return ret;
 	}
 
 	if (!obj) {
@@ -13337,9 +13341,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret == 0)
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
 
-out:
-	mutex_unlock(&dev->struct_mutex);
-
 	return ret;
 }
 
@@ -13362,7 +13363,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return;
 
-	mutex_lock(&dev->struct_mutex);
 	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state);
@@ -13371,7 +13371,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
 	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 int
-- 
2.1.0

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

[PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Maarten Lankhorst]

Make pinning and waiting a separate step, and wait for object idle
without struct_mutex held.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h           |  2 -
 drivers/gpu/drm/i915/i915_gem.c           |  6 ---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
 drivers/gpu/drm/i915/intel_display.c      | 86 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h          |  7 +--
 drivers/gpu/drm/i915/intel_fbdev.c        |  2 +-
 drivers/gpu/drm/i915/intel_overlay.c      |  6 ++-
 7 files changed, 84 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b771d0..ec72fd457499 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2982,8 +2982,6 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
 int __must_check
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     u32 alignment,
-				     struct intel_engine_cs *pipelined,
-				     struct drm_i915_gem_request **pipelined_request,
 				     const struct i915_ggtt_view *view);
 void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
 					      const struct i915_ggtt_view *view);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 46f0e83ee6ee..ab02182c47a5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3782,17 +3782,11 @@ unlock:
 int
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     u32 alignment,
-				     struct intel_engine_cs *pipelined,
-				     struct drm_i915_gem_request **pipelined_request,
 				     const struct i915_ggtt_view *view)
 {
 	u32 old_read_domains, old_write_domain;
 	int ret;
 
-	ret = i915_gem_object_sync(obj, pipelined, pipelined_request);
-	if (ret)
-		return ret;
-
 	/* Mark the pin_display early so that we account for the
 	 * display coherency whilst setting up the cache domains.
 	 */
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index a11980696595..c6bb0fc1edfb 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -84,6 +84,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
 	state = &intel_state->base;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, state);
+	intel_state->wait_req = NULL;
 
 	return state;
 }
@@ -100,6 +101,7 @@ void
 intel_plane_destroy_state(struct drm_plane *plane,
 			  struct drm_plane_state *state)
 {
+	WARN_ON(state && to_intel_plane_state(state)->wait_req);
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f046134cc9a..d817c44ee428 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2291,9 +2291,7 @@ static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv)
 int
 intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			   struct drm_framebuffer *fb,
-			   const struct drm_plane_state *plane_state,
-			   struct intel_engine_cs *pipelined,
-			   struct drm_i915_gem_request **pipelined_request)
+			   const struct drm_plane_state *plane_state)
 {
 	struct drm_device *dev = fb->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2349,8 +2347,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 */
 	intel_runtime_pm_get(dev_priv);
 
-	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
-						   pipelined_request, &view);
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment,
+						   &view);
 	if (ret)
 		goto err_pm;
 
@@ -11357,9 +11355,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	 * synchronisation, so all we want here is to pin the framebuffer
 	 * into the display plane and skip any waits.
 	 */
+	if (!mmio_flip) {
+		ret = i915_gem_object_sync(obj, ring, &request);
+		if (ret)
+			goto cleanup_pending;
+	}
+
 	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
-					 crtc->primary->state,
-					 mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request);
+					 crtc->primary->state);
 	if (ret)
 		goto cleanup_pending;
 
@@ -12993,7 +12996,10 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 				       struct drm_atomic_state *state,
 				       bool async)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane_state *plane_state;
 	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	int i, ret;
 
@@ -13006,6 +13012,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 		ret = intel_crtc_wait_for_pending_flips(crtc);
 		if (ret)
 			return ret;
+
+		if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2)
+			flush_workqueue(dev_priv->wq);
 	}
 
 	ret = i915_mutex_lock_interruptible(dev);
@@ -13013,6 +13022,37 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 		return ret;
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (!ret && !async) {
+		u32 reset_counter;
+
+		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+		mutex_unlock(&dev->struct_mutex);
+
+		for_each_plane_in_state(state, plane, plane_state, i) {
+			struct intel_plane_state *intel_plane_state =
+				to_intel_plane_state(plane_state);
+
+			if (!intel_plane_state->wait_req)
+				continue;
+
+			ret = __i915_wait_request(intel_plane_state->wait_req,
+						  reset_counter, true,
+						  NULL, NULL);
+
+			/* Swallow -EIO errors to allow updates during hw lockup. */
+			if (ret == -EIO)
+				ret = 0;
+
+			if (ret)
+				break;
+		}
+
+		if (!ret)
+			return 0;
+
+		mutex_lock(&dev->struct_mutex);
+		drm_atomic_helper_cleanup_planes(dev, state);
+	}
 
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
@@ -13039,15 +13079,17 @@ static int intel_atomic_commit(struct drm_device *dev,
 			       bool async)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
 	int ret = 0;
 	int i;
 	bool any_ms = false;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
-	if (ret)
+	if (ret) {
+		DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
 		return ret;
+	}
 
 	drm_atomic_helper_swap_state(dev, state);
 
@@ -13318,7 +13360,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		 * This should only fail upon a hung GPU, in which case we
 		 * can safely continue.
 		 */
-		if (needs_modeset(crtc_state))
+		if (needs_modeset(crtc_state) &&
+		    to_intel_plane_state(plane->state)->visible)
 			ret = i915_gem_object_wait_rendering(old_obj, true);
 
 		/* Swallow -EIO errors to allow updates during hw lockup. */
@@ -13335,11 +13378,21 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		if (ret)
 			DRM_DEBUG_KMS("failed to attach phys object\n");
 	} else {
-		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
+		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state);
 	}
 
-	if (ret == 0)
+	if (ret == 0) {
+		if (obj && obj->last_write_req &&
+		    !i915_gem_request_completed(obj->last_write_req, true)) {
+			struct intel_plane_state *plane_state =
+				to_intel_plane_state(new_state);
+
+			i915_gem_request_assign(&plane_state->wait_req,
+						obj->last_write_req);
+		}
+
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
+	}
 
 	return ret;
 }
@@ -13357,9 +13410,12 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *old_intel_state;
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
 	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
 
+	old_intel_state = to_intel_plane_state(old_state);
+
 	if (!obj && !old_obj)
 		return;
 
@@ -13371,6 +13427,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
 	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
+
+	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
+
 }
 
 int
@@ -15348,8 +15407,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(c->primary,
 						 c->primary->fb,
-						 c->primary->state,
-						 NULL, NULL);
+						 c->primary->state);
 		mutex_unlock(&dev->struct_mutex);
 		if (ret) {
 			DRM_ERROR("failed to pin boot fb on pipe %d\n",
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cfdb0f2714cd..852a0d192f82 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -270,6 +270,9 @@ struct intel_plane_state {
 	int scaler_id;
 
 	struct drm_intel_sprite_colorkey ckey;
+
+	/* async flip related structures */
+	struct drm_i915_gem_request *wait_req;
 };
 
 struct intel_initial_plane_config {
@@ -1055,9 +1058,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct drm_modeset_acquire_ctx *ctx);
 int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			       struct drm_framebuffer *fb,
-			       const struct drm_plane_state *plane_state,
-			       struct intel_engine_cs *pipelined,
-			       struct drm_i915_gem_request **pipelined_request);
+			       const struct drm_plane_state *plane_state);
 struct drm_framebuffer *
 __intel_framebuffer_create(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 65329127f0b9..51afee61ba7e 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -155,7 +155,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	}
 
 	/* Flush everything out, we'll be doing GTT only from now on */
-	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
+	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
 		goto out_fb;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 444542696a2c..1b18cc6bdbd6 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
+	ret = i915_gem_object_wait_rendering(new_bo, true);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0,
 						   &i915_ggtt_view_normal);
 	if (ret != 0)
 		return ret;
-- 
2.1.0

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

Re: [PATCH 1/5] drm/i915: Make plane fb tracking work correctly, v2.

[Ander Conselvan De Oliveira]

On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> atomic->disabled_planes is a hack that had to exist because
> prepare_fb was only called when a new fb was set. This messed
> up fb tracking in some circumstances like aborts from
> interruptible waits. As a result interruptible waiting in
> prepare_plane_fb was forbidden, but other errors could still
> cause frontbuffer tracking to be messed up.
> 
> Now that prepare_fb is always called, this hack is no longer
> required and prepare_fb may fail without consequences.
> 
> Changes since v1:
> - Clean up a few fb tracking warnings by changing plane->fb to
>   plane->state->fb.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc0086748b71..ac97af69be62 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4752,17 +4752,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> -	struct drm_plane *p;
> -
> -	/* Track fb's for any planes being disabled */
> -	drm_for_each_plane_mask(p, dev, atomic->disabled_planes) {
> -		struct intel_plane *plane = to_intel_plane(p);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(intel_fb_obj(plane->base.fb), NULL,
> -				  plane->frontbuffer_bit);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>  
>  	if (atomic->wait_for_flips)
>  		intel_crtc_wait_for_pending_flips(&crtc->base);
> @@ -11561,14 +11550,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			return ret;
>  	}
>  
> -	/*
> -	 * Disabling a plane is always okay; we just need to update
> -	 * fb tracking in a special way since cleanup_fb() won't
> -	 * get called by the plane helpers.
> -	 */
> -	if (old_plane_state->base.fb && !fb)
> -		intel_crtc->atomic.disabled_planes |= 1 << i;
> -
>  	was_visible = old_plane_state->visible;
>  	visible = to_intel_plane_state(plane_state)->visible;
>  
> @@ -13318,15 +13299,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>  	int ret = 0;
>  
> -	if (!obj)
> +	if (!obj && !old_obj)
>  		return 0;
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	if (!obj) {
> +		ret = 0;
> +	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
>  	    INTEL_INFO(dev)->cursor_needs_physical) {
>  		int align = IS_I830(dev) ? 16 * 1024 : 256;
>  		ret = i915_gem_object_attach_phys(obj, align);
> @@ -13356,17 +13339,23 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  		       const struct drm_plane_state *old_state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_state->fb);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
>  
> -	if (!obj)
> +	if (!obj && !old_obj)
>  		return;
>  
> -	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
> -	    !INTEL_INFO(dev)->cursor_needs_physical) {
> -		mutex_lock(&dev->struct_mutex);
> +	mutex_lock(&dev->struct_mutex);
> +	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
> +	    !INTEL_INFO(dev)->cursor_needs_physical))
>  		intel_unpin_fb_obj(old_state->fb, old_state);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> +
> +	/* prepare_fb aborted? */
> +	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
> +	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
> +		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);

I'm not a fan of this big condition. Would it make sense to add a new parameter to cleanup_fb() that
tells us if this is an abort clean up?

But in any case, that can be done later.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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

Re: [PATCH 1/5] drm/i915: Make plane fb tracking work correctly, v2.

[Maarten Lankhorst]

Op 14-10-15 om 14:59 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
>> atomic->disabled_planes is a hack that had to exist because
>> prepare_fb was only called when a new fb was set. This messed
>> up fb tracking in some circumstances like aborts from
>> interruptible waits. As a result interruptible waiting in
>> prepare_plane_fb was forbidden, but other errors could still
>> cause frontbuffer tracking to be messed up.
>>
>> Now that prepare_fb is always called, this hack is no longer
>> required and prepare_fb may fail without consequences.
>>
>> Changes since v1:
>> - Clean up a few fb tracking warnings by changing plane->fb to
>>   plane->state->fb.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>>  2 files changed, 18 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index fc0086748b71..ac97af69be62 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4752,17 +4752,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> -	struct drm_plane *p;
>> -
>> -	/* Track fb's for any planes being disabled */
>> -	drm_for_each_plane_mask(p, dev, atomic->disabled_planes) {
>> -		struct intel_plane *plane = to_intel_plane(p);
>> -
>> -		mutex_lock(&dev->struct_mutex);
>> -		i915_gem_track_fb(intel_fb_obj(plane->base.fb), NULL,
>> -				  plane->frontbuffer_bit);
>> -		mutex_unlock(&dev->struct_mutex);
>> -	}
>>  
>>  	if (atomic->wait_for_flips)
>>  		intel_crtc_wait_for_pending_flips(&crtc->base);
>> @@ -11561,14 +11550,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  			return ret;
>>  	}
>>  
>> -	/*
>> -	 * Disabling a plane is always okay; we just need to update
>> -	 * fb tracking in a special way since cleanup_fb() won't
>> -	 * get called by the plane helpers.
>> -	 */
>> -	if (old_plane_state->base.fb && !fb)
>> -		intel_crtc->atomic.disabled_planes |= 1 << i;
>> -
>>  	was_visible = old_plane_state->visible;
>>  	visible = to_intel_plane_state(plane_state)->visible;
>>  
>> @@ -13318,15 +13299,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  	struct drm_framebuffer *fb = new_state->fb;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>>  	int ret = 0;
>>  
>> -	if (!obj)
>> +	if (!obj && !old_obj)
>>  		return 0;
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  
>> -	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
>> +	if (!obj) {
>> +		ret = 0;
>> +	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
>>  	    INTEL_INFO(dev)->cursor_needs_physical) {
>>  		int align = IS_I830(dev) ? 16 * 1024 : 256;
>>  		ret = i915_gem_object_attach_phys(obj, align);
>> @@ -13356,17 +13339,23 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>>  		       const struct drm_plane_state *old_state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_state->fb);
>> +	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>> +	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
>>  
>> -	if (!obj)
>> +	if (!obj && !old_obj)
>>  		return;
>>  
>> -	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
>> -	    !INTEL_INFO(dev)->cursor_needs_physical) {
>> -		mutex_lock(&dev->struct_mutex);
>> +	mutex_lock(&dev->struct_mutex);
>> +	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
>> +	    !INTEL_INFO(dev)->cursor_needs_physical))
>>  		intel_unpin_fb_obj(old_state->fb, old_state);
>> -		mutex_unlock(&dev->struct_mutex);
>> -	}
>> +
>> +	/* prepare_fb aborted? */
>> +	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
>> +	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>> +		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> I'm not a fan of this big condition. Would it make sense to add a new parameter to cleanup_fb() that
> tells us if this is an abort clean up?
>
> But in any case, that can be done later.
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
>
This is only temporarily though. I plan to make unpin async in the future. When that happens cleanup_fb will only be called when aborting so it will become a moot point. :)
We could also add a force flag to i915_gem_track_fb to ignore warnings in cleanup_plane_fb later on.

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

Re: [PATCH 2/5] drm/i915: Make prepare_plane_fb fully interruptible.

[Ander Conselvan De Oliveira]

On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> Now that we agreed on not preserving framebuffers pinning is finally
> allowed to fail because of signals. Use this to make pinning
> and acquire the mutex in an interruptible way too.
> 
> Unpinning is still uninterruptible, because it happens as a cleanup
> of old state, or undoing pins after one of the pins failed.
> 
> The intel_pin_and_fence_fb_obj in page_flip will also wait interruptibly,
> and can be aborted now.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ac97af69be62..25e1eac260fd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2349,11 +2349,10 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 */
>  	intel_runtime_pm_get(dev_priv);
>  
> -	dev_priv->mm.interruptible = false;
>  	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
>  						   pipelined_request, &view);
>  	if (ret)
> -		goto err_interruptible;
> +		goto err_pm;
>  
>  	/* Install a fence for tiled scan-out. Pre-i965 always needs a
>  	 * fence, whereas 965+ only requires a fence if using
> @@ -2377,14 +2376,12 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  
>  	i915_gem_object_pin_fence(obj);
>  
> -	dev_priv->mm.interruptible = true;
>  	intel_runtime_pm_put(dev_priv);
>  	return 0;
>  
>  err_unpin:
>  	i915_gem_object_unpin_from_display_plane(obj, &view);
> -err_interruptible:
> -	dev_priv->mm.interruptible = true;
> +err_pm:
>  	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }
> @@ -13305,7 +13302,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (!obj && !old_obj)
>  		return 0;
>  
> -	mutex_lock(&dev->struct_mutex);
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return ret;
>  
>  	if (!obj) {
>  		ret = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Make prepare_plane_fb fully interruptible.

[Daniel Vetter]

On Fri, Oct 16, 2015 at 02:21:26PM +0300, Ander Conselvan De Oliveira wrote:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > Now that we agreed on not preserving framebuffers pinning is finally
> > allowed to fail because of signals. Use this to make pinning
> > and acquire the mutex in an interruptible way too.
> > 
> > Unpinning is still uninterruptible, because it happens as a cleanup
> > of old state, or undoing pins after one of the pins failed.
> > 
> > The intel_pin_and_fence_fb_obj in page_flip will also wait interruptibly,
> > and can be aborted now.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

First two patches merged to dinq.

Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index ac97af69be62..25e1eac260fd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2349,11 +2349,10 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> >  	 */
> >  	intel_runtime_pm_get(dev_priv);
> >  
> > -	dev_priv->mm.interruptible = false;
> >  	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
> >  						   pipelined_request, &view);
> >  	if (ret)
> > -		goto err_interruptible;
> > +		goto err_pm;
> >  
> >  	/* Install a fence for tiled scan-out. Pre-i965 always needs a
> >  	 * fence, whereas 965+ only requires a fence if using
> > @@ -2377,14 +2376,12 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> >  
> >  	i915_gem_object_pin_fence(obj);
> >  
> > -	dev_priv->mm.interruptible = true;
> >  	intel_runtime_pm_put(dev_priv);
> >  	return 0;
> >  
> >  err_unpin:
> >  	i915_gem_object_unpin_from_display_plane(obj, &view);
> > -err_interruptible:
> > -	dev_priv->mm.interruptible = true;
> > +err_pm:
> >  	intel_runtime_pm_put(dev_priv);
> >  	return ret;
> >  }
> > @@ -13305,7 +13302,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >  	if (!obj && !old_obj)
> >  		return 0;
> >  
> > -	mutex_lock(&dev->struct_mutex);
> > +	ret = i915_mutex_lock_interruptible(dev);
> > +	if (ret)
> > +		return ret;
> >  
> >  	if (!obj) {
> >  		ret = 0;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Ander Conselvan De Oliveira]

On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> Move it from intel_crtc_atomic_commit to prepare_plane_fb.
> Waiting is done before committing, otherwise it's too late
> to undo the changes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
>  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>  3 files changed, 62 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index f1975f267710..25a891aa3824 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  				 * but since this plane is unchanged just do
> the
>  				 * minimum required validation.
>  				 */
> -				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> -					intel_crtc->atomic.wait_for_flips =
> true;
>  				crtc_state->base.planes_changed = true;
>  			}
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 25e1eac260fd..cd651ff6c15b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
>  	drm_modeset_unlock_all(dev);
>  }
>  
> -static void
> -intel_finish_fb(struct drm_framebuffer *old_fb)
> -{
> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
> -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> -	bool was_interruptible = dev_priv->mm.interruptible;
> -	int ret;
> -
> -	/* Big Hammer, we also need to ensure that any pending
> -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> -	 * current scanout is retired before unpinning the old
> -	 * framebuffer. Note that we rely on userspace rendering
> -	 * into the buffer attached to the pipe they are waiting
> -	 * on. If not, userspace generates a GPU hang with IPEHR
> -	 * point to the MI_WAIT_FOR_EVENT.
> -	 *
> -	 * This should only fail upon a hung GPU, in which case we
> -	 * can safely continue.
> -	 */
> -	dev_priv->mm.interruptible = false;
> -	ret = i915_gem_object_wait_rendering(obj, true);
> -	dev_priv->mm.interruptible = was_interruptible;
> -
> -	WARN_ON(ret);
> -}
> -
>  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
> *intel_crtc)
>  				 work->pending_flip_obj);
>  }
>  
> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	long ret;
>  
>  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
> -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
> -				       !intel_crtc_has_pending_flip(crtc),
> -				       60*HZ) == 0)) {
> +
> +	ret = wait_event_interruptible_timeout(
> +					dev_priv->pending_flip_queue,
> +					!intel_crtc_has_pending_flip(crtc),
> +					60*HZ);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == 0) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  		spin_lock_irq(&dev->event_lock);
> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc
> *crtc)
>  		spin_unlock_irq(&dev->event_lock);
>  	}
>  
> -	if (crtc->primary->fb) {
> -		mutex_lock(&dev->struct_mutex);
> -		intel_finish_fb(crtc->primary->fb);
> -		mutex_unlock(&dev->struct_mutex);
> -	}

There is another caller of intel_crtc_wait_for_pending_flips() besides the one
touched in this patch: intel_crtc_disable_noatomic(). In your previous series
you dropped that call based on the fact that there shouldn't be any pending
flips at that point, but that patch has been dropped.

Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
keeping the wait for flips but without the work around?

> +	return 0;
>  }
>  
>  /* Program iCLKIP clock to the desired frequency */
> @@ -4750,9 +4728,6 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  
> -	if (atomic->wait_for_flips)
> -		intel_crtc_wait_for_pending_flips(&crtc->base);
> -
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
>  
> @@ -11596,7 +11571,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> -		intel_crtc->atomic.wait_for_flips = true;
>  		intel_crtc->atomic.pre_disable_primary = turn_off;
>  		intel_crtc->atomic.post_enable_primary = turn_on;
>  
> @@ -13015,6 +12989,30 @@ static int intel_atomic_check(struct drm_device *dev,
>  	return drm_atomic_helper_check_planes(state->dev, state);
>  }
>  
> +static int intel_atomic_prepare_commit(struct drm_device *dev,
> +				       struct drm_atomic_state *state,
> +				       bool async)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i, ret;
> +
> +	if (async) {
> +		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		ret = intel_crtc_wait_for_pending_flips(crtc);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = drm_atomic_helper_prepare_planes(dev, state);
> +
> +	return ret;
> +}
> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13042,12 +13040,7 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  	int i;
>  	bool any_ms = false;
>  
> -	if (async) {
> -		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> +	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret)
>  		return ret;
>  
> @@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> +	if (old_obj) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(new_state->state,
> plane->state->crtc);
> +
> +		/* Big Hammer, we also need to ensure that any pending
> +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> +		 * current scanout is retired before unpinning the old
> +		 * framebuffer. Note that we rely on userspace rendering
> +		 * into the buffer attached to the pipe they are waiting
> +		 * on. If not, userspace generates a GPU hang with IPEHR
> +		 * point to the MI_WAIT_FOR_EVENT.
> +		 *
> +		 * This should only fail upon a hung GPU, in which case we
> +		 * can safely continue.
> +		 */
> +		if (needs_modeset(crtc_state))
> +			ret = i915_gem_object_wait_rendering(old_obj, true);
> +
> +		/* Swallow -EIO errors to allow updates during hw lockup. */
> +		if (ret && ret != -EIO)
> +			goto out;

Doesn't this change the behavior of a modeset after a GPU hang? Since
mm.interruptible is true, i915_gem_check_wedge() might return -EAGAIN instead of
-EIO. Previously the modeset would continue in that scenario, but now, somewhat
contrary to the comment above, we don't continue and instead pass the -EAGAIN to
user space.

Ander


> +	}
> +
>  	if (!obj) {
>  		ret = 0;
>  	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> @@ -13321,6 +13337,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (ret == 0)
>  		i915_gem_track_fb(old_obj, obj, intel_plane
> ->frontbuffer_bit);
>  
> +out:
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a2f0b981e26c..cfdb0f2714cd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,7 +499,6 @@ struct skl_pipe_wm {
>   */
>  struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
> -	bool wait_for_flips;
>  	bool disable_fbc;
>  	bool disable_ips;
>  	bool disable_cxsr;
> @@ -1158,7 +1157,6 @@ enum intel_display_power_domain
>  intel_display_port_power_domain(struct intel_encoder *intel_encoder);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Daniel Vetter]

On Mon, Oct 19, 2015 at 04:16:53PM +0300, Ander Conselvan De Oliveira wrote:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > @@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (old_obj) {
> > +		struct drm_crtc_state *crtc_state =
> > +			drm_atomic_get_existing_crtc_state(new_state->state,
> > plane->state->crtc);
> > +
> > +		/* Big Hammer, we also need to ensure that any pending
> > +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> > +		 * current scanout is retired before unpinning the old
> > +		 * framebuffer. Note that we rely on userspace rendering
> > +		 * into the buffer attached to the pipe they are waiting
> > +		 * on. If not, userspace generates a GPU hang with IPEHR
> > +		 * point to the MI_WAIT_FOR_EVENT.
> > +		 *
> > +		 * This should only fail upon a hung GPU, in which case we
> > +		 * can safely continue.
> > +		 */
> > +		if (needs_modeset(crtc_state))
> > +			ret = i915_gem_object_wait_rendering(old_obj, true);
> > +
> > +		/* Swallow -EIO errors to allow updates during hw lockup. */
> > +		if (ret && ret != -EIO)
> > +			goto out;
> 
> Doesn't this change the behavior of a modeset after a GPU hang? Since
> mm.interruptible is true, i915_gem_check_wedge() might return -EAGAIN instead of
> -EIO. Previously the modeset would continue in that scenario, but now, somewhat
> contrary to the comment above, we don't continue and instead pass the -EAGAIN to
> user space.

It's "while the gpu hang is pending" not "after", but this change is the
hole point of making pinning interruptible. With current modeset code the
only thing we could hope for is that the reset would go through, and
otherwise we'd have to fail the modeset. Now we can correctly retry the
operation if it has run into a concurrent gpu hang/reset.

Note that we still should eat any -EIO, since modesets must continue even
if the render side is completely dead.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Maarten Lankhorst]

Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
>> Move it from intel_crtc_atomic_commit to prepare_plane_fb.
>> Waiting is done before committing, otherwise it's too late
>> to undo the changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
>>  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>>  3 files changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index f1975f267710..25a891aa3824 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  				 * but since this plane is unchanged just do
>> the
>>  				 * minimum required validation.
>>  				 */
>> -				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> -					intel_crtc->atomic.wait_for_flips =
>> true;
>>  				crtc_state->base.planes_changed = true;
>>  			}
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 25e1eac260fd..cd651ff6c15b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
>>  	drm_modeset_unlock_all(dev);
>>  }
>>  
>> -static void
>> -intel_finish_fb(struct drm_framebuffer *old_fb)
>> -{
>> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
>> -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>> -	bool was_interruptible = dev_priv->mm.interruptible;
>> -	int ret;
>> -
>> -	/* Big Hammer, we also need to ensure that any pending
>> -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>> -	 * current scanout is retired before unpinning the old
>> -	 * framebuffer. Note that we rely on userspace rendering
>> -	 * into the buffer attached to the pipe they are waiting
>> -	 * on. If not, userspace generates a GPU hang with IPEHR
>> -	 * point to the MI_WAIT_FOR_EVENT.
>> -	 *
>> -	 * This should only fail upon a hung GPU, in which case we
>> -	 * can safely continue.
>> -	 */
>> -	dev_priv->mm.interruptible = false;
>> -	ret = i915_gem_object_wait_rendering(obj, true);
>> -	dev_priv->mm.interruptible = was_interruptible;
>> -
>> -	WARN_ON(ret);
>> -}
>> -
>>  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
>> *intel_crtc)
>>  				 work->pending_flip_obj);
>>  }
>>  
>> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>> +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	long ret;
>>  
>>  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
>> -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
>> -				       !intel_crtc_has_pending_flip(crtc),
>> -				       60*HZ) == 0)) {
>> +
>> +	ret = wait_event_interruptible_timeout(
>> +					dev_priv->pending_flip_queue,
>> +					!intel_crtc_has_pending_flip(crtc),
>> +					60*HZ);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret == 0) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>>  		spin_lock_irq(&dev->event_lock);
>> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc
>> *crtc)
>>  		spin_unlock_irq(&dev->event_lock);
>>  	}
>>  
>> -	if (crtc->primary->fb) {
>> -		mutex_lock(&dev->struct_mutex);
>> -		intel_finish_fb(crtc->primary->fb);
>> -		mutex_unlock(&dev->struct_mutex);
>> -	}
> There is another caller of intel_crtc_wait_for_pending_flips() besides the one
> touched in this patch: intel_crtc_disable_noatomic(). In your previous series
> you dropped that call based on the fact that there shouldn't be any pending
> flips at that point, but that patch has been dropped.
>
> Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
> keeping the wait for flips but without the work around?
>
Yeah that would be a good idea. I'll fix up this patch.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2.9/5] drm/i915: Do not wait for flips in intel_crtc_disable_noatomic.

[Maarten Lankhorst]

Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
>> Move it from intel_crtc_atomic_commit to prepare_plane_fb.
>> Waiting is done before committing, otherwise it's too late
>> to undo the changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
>>  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>>  3 files changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index f1975f267710..25a891aa3824 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  				 * but since this plane is unchanged just do
>> the
>>  				 * minimum required validation.
>>  				 */
>> -				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> -					intel_crtc->atomic.wait_for_flips =
>> true;
>>  				crtc_state->base.planes_changed = true;
>>  			}
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 25e1eac260fd..cd651ff6c15b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
>>  	drm_modeset_unlock_all(dev);
>>  }
>>  
>> -static void
>> -intel_finish_fb(struct drm_framebuffer *old_fb)
>> -{
>> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
>> -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>> -	bool was_interruptible = dev_priv->mm.interruptible;
>> -	int ret;
>> -
>> -	/* Big Hammer, we also need to ensure that any pending
>> -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>> -	 * current scanout is retired before unpinning the old
>> -	 * framebuffer. Note that we rely on userspace rendering
>> -	 * into the buffer attached to the pipe they are waiting
>> -	 * on. If not, userspace generates a GPU hang with IPEHR
>> -	 * point to the MI_WAIT_FOR_EVENT.
>> -	 *
>> -	 * This should only fail upon a hung GPU, in which case we
>> -	 * can safely continue.
>> -	 */
>> -	dev_priv->mm.interruptible = false;
>> -	ret = i915_gem_object_wait_rendering(obj, true);
>> -	dev_priv->mm.interruptible = was_interruptible;
>> -
>> -	WARN_ON(ret);
>> -}
>> -
>>  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
>> *intel_crtc)
>>  				 work->pending_flip_obj);
>>  }
>>  
>> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>> +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	long ret;
>>  
>>  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
>> -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
>> -				       !intel_crtc_has_pending_flip(crtc),
>> -				       60*HZ) == 0)) {
>> +
>> +	ret = wait_event_interruptible_timeout(
>> +					dev_priv->pending_flip_queue,
>> +					!intel_crtc_has_pending_flip(crtc),
>> +					60*HZ);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret == 0) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>>  		spin_lock_irq(&dev->event_lock);
>> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc
>> *crtc)
>>  		spin_unlock_irq(&dev->event_lock);
>>  	}
>>  
>> -	if (crtc->primary->fb) {
>> -		mutex_lock(&dev->struct_mutex);
>> -		intel_finish_fb(crtc->primary->fb);
>> -		mutex_unlock(&dev->struct_mutex);
>> -	}
> There is another caller of intel_crtc_wait_for_pending_flips() besides the one
> touched in this patch: intel_crtc_disable_noatomic(). In your previous series
> you dropped that call based on the fact that there shouldn't be any pending
> flips at that point, but that patch has been dropped.
>
> Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
> keeping the wait for flips but without the work around?
>
Apply with --scissors

---8<------
intel_crtc_disable_noatomic is called from hw readout during init, resume and possibly reset.
During init it's too early to have a page flip queued, before suspending all page flips
should be finished and during hw reset all page flips should be removed.

It's a bug when there are pending flips here, complain with WARN_ON instead of handling it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
--
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f59029c3e577..95c59b5202c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		return;
 
 	if (to_intel_plane_state(crtc->primary->state)->visible) {
-		intel_crtc_wait_for_pending_flips(crtc);
+		WARN_ON(intel_crtc->unpin_work);
+
 		intel_pre_disable_primary(crtc);
 	}
 

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

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Ander Conselvan De Oliveira]

On Mon, 2015-10-19 at 15:30 +0200, Daniel Vetter wrote:
> On Mon, Oct 19, 2015 at 04:16:53PM +0300, Ander Conselvan De Oliveira wrote:
> > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > > @@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (old_obj) {
> > > +		struct drm_crtc_state *crtc_state =
> > > +			drm_atomic_get_existing_crtc_state(new_state
> > > ->state,
> > > plane->state->crtc);
> > > +
> > > +		/* Big Hammer, we also need to ensure that any pending
> > > +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> > > +		 * current scanout is retired before unpinning the old
> > > +		 * framebuffer. Note that we rely on userspace rendering
> > > +		 * into the buffer attached to the pipe they are waiting
> > > +		 * on. If not, userspace generates a GPU hang with IPEHR
> > > +		 * point to the MI_WAIT_FOR_EVENT.
> > > +		 *
> > > +		 * This should only fail upon a hung GPU, in which case
> > > we
> > > +		 * can safely continue.
> > > +		 */
> > > +		if (needs_modeset(crtc_state))
> > > +			ret = i915_gem_object_wait_rendering(old_obj,
> > > true);
> > > +
> > > +		/* Swallow -EIO errors to allow updates during hw lockup.
> > > */
> > > +		if (ret && ret != -EIO)
> > > +			goto out;
> > 
> > Doesn't this change the behavior of a modeset after a GPU hang? Since
> > mm.interruptible is true, i915_gem_check_wedge() might return -EAGAIN
> > instead of
> > -EIO. Previously the modeset would continue in that scenario, but now,
> > somewhat
> > contrary to the comment above, we don't continue and instead pass the 
> > -EAGAIN to
> > user space.
> 
> It's "while the gpu hang is pending" not "after", but this change is the
> hole point of making pinning interruptible. With current modeset code the
> only thing we could hope for is that the reset would go through, and
> otherwise we'd have to fail the modeset. Now we can correctly retry the
> operation if it has run into a concurrent gpu hang/reset.

So in that case should user space retry the modeset? I don't think it does that
at moment, at least weston and xf86-video-intel don't. I'm not sure how big of a
deal that is, though, since it is an unlikely corner case.

Ander

> Note that we still should eat any -EIO, since modesets must continue even
> if the render side is completely dead.
> -Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:38:41AM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-10-19 at 15:30 +0200, Daniel Vetter wrote:
> > On Mon, Oct 19, 2015 at 04:16:53PM +0300, Ander Conselvan De Oliveira wrote:
> > > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > > > @@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > +	if (old_obj) {
> > > > +		struct drm_crtc_state *crtc_state =
> > > > +			drm_atomic_get_existing_crtc_state(new_state
> > > > ->state,
> > > > plane->state->crtc);
> > > > +
> > > > +		/* Big Hammer, we also need to ensure that any pending
> > > > +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> > > > +		 * current scanout is retired before unpinning the old
> > > > +		 * framebuffer. Note that we rely on userspace rendering
> > > > +		 * into the buffer attached to the pipe they are waiting
> > > > +		 * on. If not, userspace generates a GPU hang with IPEHR
> > > > +		 * point to the MI_WAIT_FOR_EVENT.
> > > > +		 *
> > > > +		 * This should only fail upon a hung GPU, in which case
> > > > we
> > > > +		 * can safely continue.
> > > > +		 */
> > > > +		if (needs_modeset(crtc_state))
> > > > +			ret = i915_gem_object_wait_rendering(old_obj,
> > > > true);
> > > > +
> > > > +		/* Swallow -EIO errors to allow updates during hw lockup.
> > > > */
> > > > +		if (ret && ret != -EIO)
> > > > +			goto out;
> > > 
> > > Doesn't this change the behavior of a modeset after a GPU hang? Since
> > > mm.interruptible is true, i915_gem_check_wedge() might return -EAGAIN
> > > instead of
> > > -EIO. Previously the modeset would continue in that scenario, but now,
> > > somewhat
> > > contrary to the comment above, we don't continue and instead pass the 
> > > -EAGAIN to
> > > user space.
> > 
> > It's "while the gpu hang is pending" not "after", but this change is the
> > hole point of making pinning interruptible. With current modeset code the
> > only thing we could hope for is that the reset would go through, and
> > otherwise we'd have to fail the modeset. Now we can correctly retry the
> > operation if it has run into a concurrent gpu hang/reset.
> 
> So in that case should user space retry the modeset? I don't think it does that
> at moment, at least weston and xf86-video-intel don't. I'm not sure how big of a
> deal that is, though, since it is an unlikely corner case.

Every drm ioctl goes through drmIoctl in libdrm, which does this
restarting. If not then that's a userspace bug, since this is a core bit
of the drm ABI design.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2.9/5] drm/i915: Do not wait for flips in intel_crtc_disable_noatomic.

[Ander Conselvan De Oliveira]

On Mon, 2015-10-19 at 17:09 +0200, Maarten Lankhorst wrote:
> Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:
> > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > > Move it from intel_crtc_atomic_commit to prepare_plane_fb.
> > > Waiting is done before committing, otherwise it's too late
> > > to undo the changes.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
> > >  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++----------
> > > ----
> > > -
> > >  drivers/gpu/drm/i915/intel_drv.h     |   2 -
> > >  3 files changed, 62 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > index f1975f267710..25a891aa3824 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> > >  				 * but since this plane is unchanged just
> > > do
> > > the
> > >  				 * minimum required validation.
> > >  				 */
> > > -				if (plane->type ==
> > > DRM_PLANE_TYPE_PRIMARY)
> > > -					intel_crtc->atomic.wait_for_flips
> > > =
> > > true;
> > >  				crtc_state->base.planes_changed = true;
> > >  			}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 25e1eac260fd..cd651ff6c15b 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
> > >  	drm_modeset_unlock_all(dev);
> > >  }
> > >  
> > > -static void
> > > -intel_finish_fb(struct drm_framebuffer *old_fb)
> > > -{
> > > -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
> > > -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > > -	bool was_interruptible = dev_priv->mm.interruptible;
> > > -	int ret;
> > > -
> > > -	/* Big Hammer, we also need to ensure that any pending
> > > -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> > > -	 * current scanout is retired before unpinning the old
> > > -	 * framebuffer. Note that we rely on userspace rendering
> > > -	 * into the buffer attached to the pipe they are waiting
> > > -	 * on. If not, userspace generates a GPU hang with IPEHR
> > > -	 * point to the MI_WAIT_FOR_EVENT.
> > > -	 *
> > > -	 * This should only fail upon a hung GPU, in which case we
> > > -	 * can safely continue.
> > > -	 */
> > > -	dev_priv->mm.interruptible = false;
> > > -	ret = i915_gem_object_wait_rendering(obj, true);
> > > -	dev_priv->mm.interruptible = was_interruptible;
> > > -
> > > -	WARN_ON(ret);
> > > -}
> > > -
> > >  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > >  {
> > >  	struct drm_device *dev = crtc->dev;
> > > @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
> > > *intel_crtc)
> > >  				 work->pending_flip_obj);
> > >  }
> > >  
> > > -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> > > +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> > >  {
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	long ret;
> > >  
> > >  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
> > > -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
> > > -				      
> > >  !intel_crtc_has_pending_flip(crtc),
> > > -				       60*HZ) == 0)) {
> > > +
> > > +	ret = wait_event_interruptible_timeout(
> > > +					dev_priv->pending_flip_queue,
> > > +					!intel_crtc_has_pending_flip(crtc
> > > ),
> > > +					60*HZ);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (ret == 0) {
> > >  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  
> > >  		spin_lock_irq(&dev->event_lock);
> > > @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct
> > > drm_crtc
> > > *crtc)
> > >  		spin_unlock_irq(&dev->event_lock);
> > >  	}
> > >  
> > > -	if (crtc->primary->fb) {
> > > -		mutex_lock(&dev->struct_mutex);
> > > -		intel_finish_fb(crtc->primary->fb);
> > > -		mutex_unlock(&dev->struct_mutex);
> > > -	}
> > There is another caller of intel_crtc_wait_for_pending_flips() besides the
> > one
> > touched in this patch: intel_crtc_disable_noatomic(). In your previous
> > series
> > you dropped that call based on the fact that there shouldn't be any pending
> > flips at that point, but that patch has been dropped.
> > 
> > Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
> > keeping the wait for flips but without the work around?
> > 
> Apply with --scissors
> 
> ---8<------
> intel_crtc_disable_noatomic is called from hw readout during init, resume and
> possibly reset.
> During init it's too early to have a page flip queued, before suspending all
> page flips
> should be finished and during hw reset all page flips should be removed.
> 
> It's a bug when there are pending flips here, complain with WARN_ON instead of
> handling it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> --
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f59029c3e577..95c59b5202c5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
> *crtc)
>  		return;
>  
>  	if (to_intel_plane_state(crtc->primary->state)->visible) {
> -		intel_crtc_wait_for_pending_flips(crtc);
> +		WARN_ON(intel_crtc->unpin_work);
> +
>  		intel_pre_disable_primary(crtc);
>  	}
>  
> 

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Ander Conselvan De Oliveira]

On Tue, 2015-10-20 at 10:10 +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 10:38:41AM +0300, Ander Conselvan De Oliveira wrote:
> > On Mon, 2015-10-19 at 15:30 +0200, Daniel Vetter wrote:
> > > On Mon, Oct 19, 2015 at 04:16:53PM +0300, Ander Conselvan De Oliveira
> > > wrote:
> > > > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> > > > > @@ -13306,6 +13299,29 @@ intel_prepare_plane_fb(struct drm_plane
> > > > > *plane,
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > +	if (old_obj) {
> > > > > +		struct drm_crtc_state *crtc_state =
> > > > > +			drm_atomic_get_existing_crtc_state(new_state
> > > > > ->state,
> > > > > plane->state->crtc);
> > > > > +
> > > > > +		/* Big Hammer, we also need to ensure that any
> > > > > pending
> > > > > +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on
> > > > > the
> > > > > +		 * current scanout is retired before unpinning the
> > > > > old
> > > > > +		 * framebuffer. Note that we rely on userspace
> > > > > rendering
> > > > > +		 * into the buffer attached to the pipe they are
> > > > > waiting
> > > > > +		 * on. If not, userspace generates a GPU hang with
> > > > > IPEHR
> > > > > +		 * point to the MI_WAIT_FOR_EVENT.
> > > > > +		 *
> > > > > +		 * This should only fail upon a hung GPU, in which
> > > > > case
> > > > > we
> > > > > +		 * can safely continue.
> > > > > +		 */
> > > > > +		if (needs_modeset(crtc_state))
> > > > > +			ret = i915_gem_object_wait_rendering(old_obj,
> > > > > true);
> > > > > +
> > > > > +		/* Swallow -EIO errors to allow updates during hw
> > > > > lockup.
> > > > > */
> > > > > +		if (ret && ret != -EIO)
> > > > > +			goto out;
> > > > 
> > > > Doesn't this change the behavior of a modeset after a GPU hang? Since
> > > > mm.interruptible is true, i915_gem_check_wedge() might return -EAGAIN
> > > > instead of
> > > > -EIO. Previously the modeset would continue in that scenario, but now,
> > > > somewhat
> > > > contrary to the comment above, we don't continue and instead pass the 
> > > > -EAGAIN to
> > > > user space.
> > > 
> > > It's "while the gpu hang is pending" not "after", but this change is the
> > > hole point of making pinning interruptible. With current modeset code the
> > > only thing we could hope for is that the reset would go through, and
> > > otherwise we'd have to fail the modeset. Now we can correctly retry the
> > > operation if it has run into a concurrent gpu hang/reset.
> > 
> > So in that case should user space retry the modeset? I don't think it does
> > that
> > at moment, at least weston and xf86-video-intel don't. I'm not sure how big
> > of a
> > deal that is, though, since it is an unlikely corner case.
> 
> Every drm ioctl goes through drmIoctl in libdrm, which does this
> restarting. If not then that's a userspace bug, since this is a core bit
> of the drm ABI design.

Ah, true, completely forgot about that.


With the new 2.9 patch, I don't see any other issues with this. I'm happy to
give my r-b, but I think someone else that knows this stuff a bit better should
at least ack the patch.

Ander

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

Re: [PATCH 2.9/5] drm/i915: Do not wait for flips in intel_crtc_disable_noatomic.

[Daniel Vetter]

On Mon, Oct 19, 2015 at 05:09:23PM +0200, Maarten Lankhorst wrote:
> Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:
> > On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
> >> Move it from intel_crtc_atomic_commit to prepare_plane_fb.
> >> Waiting is done before committing, otherwise it's too late
> >> to undo the changes.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
> >>  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------
> >> -
> >>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
> >>  3 files changed, 62 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >> index f1975f267710..25a891aa3824 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> >>  				 * but since this plane is unchanged just do
> >> the
> >>  				 * minimum required validation.
> >>  				 */
> >> -				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >> -					intel_crtc->atomic.wait_for_flips =
> >> true;
> >>  				crtc_state->base.planes_changed = true;
> >>  			}
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 25e1eac260fd..cd651ff6c15b 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
> >>  	drm_modeset_unlock_all(dev);
> >>  }
> >>  
> >> -static void
> >> -intel_finish_fb(struct drm_framebuffer *old_fb)
> >> -{
> >> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
> >> -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >> -	bool was_interruptible = dev_priv->mm.interruptible;
> >> -	int ret;
> >> -
> >> -	/* Big Hammer, we also need to ensure that any pending
> >> -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> >> -	 * current scanout is retired before unpinning the old
> >> -	 * framebuffer. Note that we rely on userspace rendering
> >> -	 * into the buffer attached to the pipe they are waiting
> >> -	 * on. If not, userspace generates a GPU hang with IPEHR
> >> -	 * point to the MI_WAIT_FOR_EVENT.
> >> -	 *
> >> -	 * This should only fail upon a hung GPU, in which case we
> >> -	 * can safely continue.
> >> -	 */
> >> -	dev_priv->mm.interruptible = false;
> >> -	ret = i915_gem_object_wait_rendering(obj, true);
> >> -	dev_priv->mm.interruptible = was_interruptible;
> >> -
> >> -	WARN_ON(ret);
> >> -}
> >> -
> >>  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
> >> *intel_crtc)
> >>  				 work->pending_flip_obj);
> >>  }
> >>  
> >> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> >> +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	long ret;
> >>  
> >>  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
> >> -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
> >> -				       !intel_crtc_has_pending_flip(crtc),
> >> -				       60*HZ) == 0)) {
> >> +
> >> +	ret = wait_event_interruptible_timeout(
> >> +					dev_priv->pending_flip_queue,
> >> +					!intel_crtc_has_pending_flip(crtc),
> >> +					60*HZ);
> >> +
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	if (ret == 0) {
> >>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  
> >>  		spin_lock_irq(&dev->event_lock);
> >> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc
> >> *crtc)
> >>  		spin_unlock_irq(&dev->event_lock);
> >>  	}
> >>  
> >> -	if (crtc->primary->fb) {
> >> -		mutex_lock(&dev->struct_mutex);
> >> -		intel_finish_fb(crtc->primary->fb);
> >> -		mutex_unlock(&dev->struct_mutex);
> >> -	}
> > There is another caller of intel_crtc_wait_for_pending_flips() besides the one
> > touched in this patch: intel_crtc_disable_noatomic(). In your previous series
> > you dropped that call based on the fact that there shouldn't be any pending
> > flips at that point, but that patch has been dropped.
> >
> > Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
> > keeping the wait for flips but without the work around?
> >
> Apply with --scissors
> 
> ---8<------
> intel_crtc_disable_noatomic is called from hw readout during init, resume and possibly reset.
> During init it's too early to have a page flip queued, before suspending all page flips
> should be finished and during hw reset all page flips should be removed.
> 
> It's a bug when there are pending flips here, complain with WARN_ON instead of handling it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> --
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f59029c3e577..95c59b5202c5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6225,7 +6225,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  		return;
>  
>  	if (to_intel_plane_state(crtc->primary->state)->visible) {
> -		intel_crtc_wait_for_pending_flips(crtc);
> +		WARN_ON(intel_crtc->unpin_work);
> +
>  		intel_pre_disable_primary(crtc);
>  	}
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Change locking for struct_mutex.

[Matt Roper]

On Wed, Sep 23, 2015 at 01:27:11PM +0200, Maarten Lankhorst wrote:
> Only acquire the struct_mutex once, and interruptibly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Your headline/commit message seem a bit sparse here...you may want to
make it clear that this refers to framebuffer preparation/cleanup for
atomic commits.  The i915 driver in general still has a bunch of of
struct_mutex usage in other places that isn't touched by this patch.

Effectively you're changing logic from:

        loop {
                grab mutex()
                stuff
                drop mutex()
        }

to

        grab mutex()
        loop {
                stuff
        }
        drop mutex()

The code change itself looks fine to me, so with an updated commit
message you can consider this

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cd651ff6c15b..2f046134cc9a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13008,8 +13008,13 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  			return ret;
>  	}
>  
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>  
> +	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
>  
> @@ -13108,7 +13113,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	/* FIXME: add subpixel order */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	if (any_ms)
>  		intel_modeset_check_state(dev, state);
> @@ -13295,10 +13303,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (!obj && !old_obj)
>  		return 0;
>  
> -	ret = i915_mutex_lock_interruptible(dev);
> -	if (ret)
> -		return ret;
> -
>  	if (old_obj) {
>  		struct drm_crtc_state *crtc_state =
>  			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
> @@ -13319,7 +13323,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  
>  		/* Swallow -EIO errors to allow updates during hw lockup. */
>  		if (ret && ret != -EIO)
> -			goto out;
> +			return ret;
>  	}
>  
>  	if (!obj) {
> @@ -13337,9 +13341,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (ret == 0)
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
>  
> -out:
> -	mutex_unlock(&dev->struct_mutex);
> -
>  	return ret;
>  }
>  
> @@ -13362,7 +13363,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if (!obj && !old_obj)
>  		return;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
>  	    !INTEL_INFO(dev)->cursor_needs_physical))
>  		intel_unpin_fb_obj(old_state->fb, old_state);
> @@ -13371,7 +13371,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
>  	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  int
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Matt Roper]

On Wed, Sep 23, 2015 at 01:27:12PM +0200, Maarten Lankhorst wrote:
> Make pinning and waiting a separate step, and wait for object idle
> without struct_mutex held.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h           |  2 -
>  drivers/gpu/drm/i915/i915_gem.c           |  6 ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
>  drivers/gpu/drm/i915/intel_display.c      | 86 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h          |  7 +--
>  drivers/gpu/drm/i915/intel_fbdev.c        |  2 +-
>  drivers/gpu/drm/i915/intel_overlay.c      |  6 ++-
>  7 files changed, 84 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b771d0..ec72fd457499 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2982,8 +2982,6 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>  int __must_check
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     u32 alignment,
> -				     struct intel_engine_cs *pipelined,
> -				     struct drm_i915_gem_request **pipelined_request,
>  				     const struct i915_ggtt_view *view);
>  void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>  					      const struct i915_ggtt_view *view);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 46f0e83ee6ee..ab02182c47a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3782,17 +3782,11 @@ unlock:
>  int
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  				     u32 alignment,
> -				     struct intel_engine_cs *pipelined,
> -				     struct drm_i915_gem_request **pipelined_request,
>  				     const struct i915_ggtt_view *view)
>  {
>  	u32 old_read_domains, old_write_domain;
>  	int ret;
>  
> -	ret = i915_gem_object_sync(obj, pipelined, pipelined_request);
> -	if (ret)
> -		return ret;
> -
>  	/* Mark the pin_display early so that we account for the
>  	 * display coherency whilst setting up the cache domains.
>  	 */
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index a11980696595..c6bb0fc1edfb 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -84,6 +84,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
>  	state = &intel_state->base;
>  
>  	__drm_atomic_helper_plane_duplicate_state(plane, state);
> +	intel_state->wait_req = NULL;
>  
>  	return state;
>  }
> @@ -100,6 +101,7 @@ void
>  intel_plane_destroy_state(struct drm_plane *plane,
>  			  struct drm_plane_state *state)
>  {
> +	WARN_ON(state && to_intel_plane_state(state)->wait_req);
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2f046134cc9a..d817c44ee428 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2291,9 +2291,7 @@ static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv)
>  int
>  intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			   struct drm_framebuffer *fb,
> -			   const struct drm_plane_state *plane_state,
> -			   struct intel_engine_cs *pipelined,
> -			   struct drm_i915_gem_request **pipelined_request)
> +			   const struct drm_plane_state *plane_state)
>  {
>  	struct drm_device *dev = fb->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2349,8 +2347,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 */
>  	intel_runtime_pm_get(dev_priv);
>  
> -	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
> -						   pipelined_request, &view);
> +	ret = i915_gem_object_pin_to_display_plane(obj, alignment,
> +						   &view);

The pin to display plane (and hence the wait) happens inside
intel_runtime_pm_get/put() in the current code.  When you pull the wait
out to the various callsites, it isn't holding runtime pm anymore (at
least not in a way that's obvious to me).  Can this be a problem?
Neither runtime PM nor GEM internals are something I'm terribly familiar
with, so you might want to get an ack from someone like Paulo or Chris?


>  	if (ret)
>  		goto err_pm;
>  
> @@ -11357,9 +11355,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	 * synchronisation, so all we want here is to pin the framebuffer
>  	 * into the display plane and skip any waits.
>  	 */
> +	if (!mmio_flip) {
> +		ret = i915_gem_object_sync(obj, ring, &request);
> +		if (ret)
> +			goto cleanup_pending;
> +	}
> +
>  	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
> -					 crtc->primary->state,
> -					 mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request);
> +					 crtc->primary->state);

Probably a dumb question from someone who isn't familiar with GEM
handling...the comment above the lines you added here says we just want
to pin and skip any waits when doing MMIO flips (which matches the logic
of your code change).  However it looks to me like the original code was
still doing a wait, just with a potentially different ring parameter.
What am I missing here?

>  	if (ret)
>  		goto cleanup_pending;
>  
> @@ -12993,7 +12996,10 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  				       struct drm_atomic_state *state,
>  				       bool async)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane_state *plane_state;
>  	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
>  	struct drm_crtc *crtc;
>  	int i, ret;
>  
> @@ -13006,6 +13012,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  		ret = intel_crtc_wait_for_pending_flips(crtc);
>  		if (ret)
>  			return ret;
> +
> +		if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2)
> +			flush_workqueue(dev_priv->wq);
>  	}
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> @@ -13013,6 +13022,37 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  		return ret;
>  
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
> +	if (!ret && !async) {
> +		u32 reset_counter;
> +
> +		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> +		mutex_unlock(&dev->struct_mutex);
> +
> +		for_each_plane_in_state(state, plane, plane_state, i) {
> +			struct intel_plane_state *intel_plane_state =
> +				to_intel_plane_state(plane_state);
> +
> +			if (!intel_plane_state->wait_req)
> +				continue;
> +
> +			ret = __i915_wait_request(intel_plane_state->wait_req,
> +						  reset_counter, true,
> +						  NULL, NULL);
> +
> +			/* Swallow -EIO errors to allow updates during hw lockup. */
> +			if (ret == -EIO)
> +				ret = 0;
> +
> +			if (ret)
> +				break;
> +		}
> +
> +		if (!ret)
> +			return 0;
> +
> +		mutex_lock(&dev->struct_mutex);
> +		drm_atomic_helper_cleanup_planes(dev, state);
> +	}
>  
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
> @@ -13039,15 +13079,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			       bool async)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
>  	int ret = 0;
>  	int i;
>  	bool any_ms = false;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
> -	if (ret)
> +	if (ret) {
> +		DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
>  		return ret;
> +	}
>  
>  	drm_atomic_helper_swap_state(dev, state);
>  
> @@ -13318,7 +13360,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		 * This should only fail upon a hung GPU, in which case we
>  		 * can safely continue.
>  		 */
> -		if (needs_modeset(crtc_state))
> +		if (needs_modeset(crtc_state) &&
> +		    to_intel_plane_state(plane->state)->visible)
>  			ret = i915_gem_object_wait_rendering(old_obj, true);
>  
>  		/* Swallow -EIO errors to allow updates during hw lockup. */
> @@ -13335,11 +13378,21 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		if (ret)
>  			DRM_DEBUG_KMS("failed to attach phys object\n");
>  	} else {
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
> +		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state);
>  	}
>  
> -	if (ret == 0)
> +	if (ret == 0) {
> +		if (obj && obj->last_write_req &&
> +		    !i915_gem_request_completed(obj->last_write_req, true)) {
> +			struct intel_plane_state *plane_state =
> +				to_intel_plane_state(new_state);
> +
> +			i915_gem_request_assign(&plane_state->wait_req,
> +						obj->last_write_req);
> +		}
> +
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> +	}
>  
>  	return ret;
>  }
> @@ -13357,9 +13410,12 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *old_intel_state;
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
>  
> +	old_intel_state = to_intel_plane_state(old_state);
> +
>  	if (!obj && !old_obj)
>  		return;
>  
> @@ -13371,6 +13427,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
>  	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> +
> +	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
> +
>  }
>  
>  int
> @@ -15348,8 +15407,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		mutex_lock(&dev->struct_mutex);
>  		ret = intel_pin_and_fence_fb_obj(c->primary,
>  						 c->primary->fb,
> -						 c->primary->state,
> -						 NULL, NULL);
> +						 c->primary->state);
>  		mutex_unlock(&dev->struct_mutex);
>  		if (ret) {
>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cfdb0f2714cd..852a0d192f82 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -270,6 +270,9 @@ struct intel_plane_state {
>  	int scaler_id;
>  
>  	struct drm_intel_sprite_colorkey ckey;
> +
> +	/* async flip related structures */
> +	struct drm_i915_gem_request *wait_req;
>  };
>  
>  struct intel_initial_plane_config {
> @@ -1055,9 +1058,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct drm_modeset_acquire_ctx *ctx);
>  int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			       struct drm_framebuffer *fb,
> -			       const struct drm_plane_state *plane_state,
> -			       struct intel_engine_cs *pipelined,
> -			       struct drm_i915_gem_request **pipelined_request);
> +			       const struct drm_plane_state *plane_state);
>  struct drm_framebuffer *
>  __intel_framebuffer_create(struct drm_device *dev,
>  			   struct drm_mode_fb_cmd2 *mode_cmd,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 65329127f0b9..51afee61ba7e 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -155,7 +155,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	}
>  
>  	/* Flush everything out, we'll be doing GTT only from now on */
> -	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> +	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
>  		goto out_fb;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 444542696a2c..1b18cc6bdbd6 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  	if (ret != 0)
>  		return ret;
>  
> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
> +	ret = i915_gem_object_wait_rendering(new_bo, true);

Again, I'm not super familiar with GEM internals...can this be a
behavior change from the previous code?  Originally the pin_to_display
plane function would have passed (obj->base.pending_write_domain == 0)
as the second parameter here (readonly), but you're unconditionally
passing true.  Can there not be pending writes against this object?


Overall I feel like this patch goes a bit too far outside my area of
expertise for me to serve as the final reviewer for it.  Maybe it would
be good to have someone like Chris or Ville take a look since they have
a lot more experience in this area.


Matt

> +	if (ret)
> +		return ret;
> +
> +	ret = i915_gem_object_pin_to_display_plane(new_bo, 0,
>  						   &i915_ggtt_view_normal);
>  	if (ret != 0)
>  		return ret;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/5] drm/i915: Change locking for struct_mutex, v2.

[Maarten Lankhorst]

struct_mutex is being locked for every plane in intel_prepare_plane_fb and
intel_cleanup_plane_fb. This can be optimized by acquiring struct_mutex first
before calling the atomic helpers. This way the lock only needs to be acquired
twice in ->atomic_commit(). Once for pinning new framebuffers at the start,
the second time for unpinning old framebuffer.

Changes since v1:
- Use mutex_lock_interruptible instead of i915 variant,
  to prevent a deadlock when atomic_commit is called from the reset code.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
Does this look better?

 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 36e7e29ea266..13aaae38f7f8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13165,8 +13165,13 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 			return ret;
 	}
 
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 
+	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
 
@@ -13268,7 +13273,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 	/* FIXME: add subpixel order */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (any_ms)
 		intel_modeset_check_state(dev, state);
@@ -13453,10 +13461,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return 0;
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
-
 	if (old_obj) {
 		struct drm_crtc_state *crtc_state =
 			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
@@ -13477,7 +13481,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 		/* Swallow -EIO errors to allow updates during hw lockup. */
 		if (ret && ret != -EIO)
-			goto out;
+			return ret;
 	}
 
 	if (!obj) {
@@ -13495,9 +13499,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret == 0)
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
 
-out:
-	mutex_unlock(&dev->struct_mutex);
-
 	return ret;
 }
 
@@ -13520,7 +13521,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return;
 
-	mutex_lock(&dev->struct_mutex);
 	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state);
@@ -13529,7 +13529,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
 	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 int
-- 
2.1.0


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

Re: [PATCH v2 4/5] drm/i915: Change locking for struct_mutex, v2.

[Chris Wilson]

On Mon, Nov 02, 2015 at 01:57:59PM +0100, Maarten Lankhorst wrote:
> struct_mutex is being locked for every plane in intel_prepare_plane_fb and
> intel_cleanup_plane_fb. This can be optimized by acquiring struct_mutex first
> before calling the atomic helpers. This way the lock only needs to be acquired
> twice in ->atomic_commit(). Once for pinning new framebuffers at the start,
> the second time for unpinning old framebuffer.

A little explanation that you move the locking into the caller would
help clarify the patch.

> @@ -13453,10 +13461,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> @@ -13520,7 +13521,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,

Please document that you now expect both of these functions to be called
with struct_mutex held. Also is there any opportunity to reduce the
struct_mutex lock time?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Maarten Lankhorst]

Op 29-10-15 om 01:30 schreef Matt Roper:
> On Wed, Sep 23, 2015 at 01:27:12PM +0200, Maarten Lankhorst wrote:
>> Make pinning and waiting a separate step, and wait for object idle
>> without struct_mutex held.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h           |  2 -
>>  drivers/gpu/drm/i915/i915_gem.c           |  6 ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
>>  drivers/gpu/drm/i915/intel_display.c      | 86 ++++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_drv.h          |  7 +--
>>  drivers/gpu/drm/i915/intel_fbdev.c        |  2 +-
>>  drivers/gpu/drm/i915/intel_overlay.c      |  6 ++-
>>  7 files changed, 84 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 3bf8a9b771d0..ec72fd457499 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2982,8 +2982,6 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>>  int __must_check
>>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>  				     u32 alignment,
>> -				     struct intel_engine_cs *pipelined,
>> -				     struct drm_i915_gem_request **pipelined_request,
>>  				     const struct i915_ggtt_view *view);
>>  void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>>  					      const struct i915_ggtt_view *view);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 46f0e83ee6ee..ab02182c47a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3782,17 +3782,11 @@ unlock:
>>  int
>>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>  				     u32 alignment,
>> -				     struct intel_engine_cs *pipelined,
>> -				     struct drm_i915_gem_request **pipelined_request,
>>  				     const struct i915_ggtt_view *view)
>>  {
>>  	u32 old_read_domains, old_write_domain;
>>  	int ret;
>>  
>> -	ret = i915_gem_object_sync(obj, pipelined, pipelined_request);
>> -	if (ret)
>> -		return ret;
>> -
>>  	/* Mark the pin_display early so that we account for the
>>  	 * display coherency whilst setting up the cache domains.
>>  	 */
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index a11980696595..c6bb0fc1edfb 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -84,6 +84,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
>>  	state = &intel_state->base;
>>  
>>  	__drm_atomic_helper_plane_duplicate_state(plane, state);
>> +	intel_state->wait_req = NULL;
>>  
>>  	return state;
>>  }
>> @@ -100,6 +101,7 @@ void
>>  intel_plane_destroy_state(struct drm_plane *plane,
>>  			  struct drm_plane_state *state)
>>  {
>> +	WARN_ON(state && to_intel_plane_state(state)->wait_req);
>>  	drm_atomic_helper_plane_destroy_state(plane, state);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2f046134cc9a..d817c44ee428 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2291,9 +2291,7 @@ static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv)
>>  int
>>  intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>  			   struct drm_framebuffer *fb,
>> -			   const struct drm_plane_state *plane_state,
>> -			   struct intel_engine_cs *pipelined,
>> -			   struct drm_i915_gem_request **pipelined_request)
>> +			   const struct drm_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = fb->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -2349,8 +2347,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>  	 */
>>  	intel_runtime_pm_get(dev_priv);
>>  
>> -	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
>> -						   pipelined_request, &view);
>> +	ret = i915_gem_object_pin_to_display_plane(obj, alignment,
>> +						   &view);
> The pin to display plane (and hence the wait) happens inside
> intel_runtime_pm_get/put() in the current code.  When you pull the wait
> out to the various callsites, it isn't holding runtime pm anymore (at
> least not in a way that's obvious to me).  Can this be a problem?
> Neither runtime PM nor GEM internals are something I'm terribly familiar
> with, so you might want to get an ack from someone like Paulo or Chris?
I don't think the GPU
>
>>  	if (ret)
>>  		goto err_pm;
>>  
>> @@ -11357,9 +11355,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>  	 * synchronisation, so all we want here is to pin the framebuffer
>>  	 * into the display plane and skip any waits.
>>  	 */
>> +	if (!mmio_flip) {
>> +		ret = i915_gem_object_sync(obj, ring, &request);
>> +		if (ret)
>> +			goto cleanup_pending;
>> +	}
>> +
>>  	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
>> -					 crtc->primary->state,
>> -					 mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request);
>> +					 crtc->primary->state);
> Probably a dumb question from someone who isn't familiar with GEM
> handling...the comment above the lines you added here says we just want
> to pin and skip any waits when doing MMIO flips (which matches the logic
> of your code change).  However it looks to me like the original code was
> still doing a wait, just with a potentially different ring parameter.
> What am I missing here?
A layer of obfuscation.

When doing mmio flips it was using i915_gem_request_get_ring because passing NULL would result in a CPU wait.
If it did wait with mmio flips it was a bug, because intel_mmio_flip_work_func does the waiting for mmio flips.

>>  	if (ret)
>>  		goto cleanup_pending;
>>  
>> @@ -12993,7 +12996,10 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>>  				       struct drm_atomic_state *state,
>>  				       bool async)
>>  {
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_plane_state *plane_state;
>>  	struct drm_crtc_state *crtc_state;
>> +	struct drm_plane *plane;
>>  	struct drm_crtc *crtc;
>>  	int i, ret;
>>  
>> @@ -13006,6 +13012,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>>  		ret = intel_crtc_wait_for_pending_flips(crtc);
>>  		if (ret)
>>  			return ret;
>> +
>> +		if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2)
>> +			flush_workqueue(dev_priv->wq);
>>  	}
>>  
>>  	ret = i915_mutex_lock_interruptible(dev);
>> @@ -13013,6 +13022,37 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>>  		return ret;
>>  
>>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>> +	if (!ret && !async) {
>> +		u32 reset_counter;
>> +
>> +		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>> +		mutex_unlock(&dev->struct_mutex);
>> +
>> +		for_each_plane_in_state(state, plane, plane_state, i) {
>> +			struct intel_plane_state *intel_plane_state =
>> +				to_intel_plane_state(plane_state);
>> +
>> +			if (!intel_plane_state->wait_req)
>> +				continue;
>> +
>> +			ret = __i915_wait_request(intel_plane_state->wait_req,
>> +						  reset_counter, true,
>> +						  NULL, NULL);
>> +
>> +			/* Swallow -EIO errors to allow updates during hw lockup. */
>> +			if (ret == -EIO)
>> +				ret = 0;
>> +
>> +			if (ret)
>> +				break;
>> +		}
>> +
>> +		if (!ret)
>> +			return 0;
>> +
>> +		mutex_lock(&dev->struct_mutex);
>> +		drm_atomic_helper_cleanup_planes(dev, state);
>> +	}
>>  
>>  	mutex_unlock(&dev->struct_mutex);
>>  	return ret;
>> @@ -13039,15 +13079,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  			       bool async)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc *crtc;
>>  	int ret = 0;
>>  	int i;
>>  	bool any_ms = false;
>>  
>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>> -	if (ret)
>> +	if (ret) {
>> +		DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
>>  		return ret;
>> +	}
>>  
>>  	drm_atomic_helper_swap_state(dev, state);
>>  
>> @@ -13318,7 +13360,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  		 * This should only fail upon a hung GPU, in which case we
>>  		 * can safely continue.
>>  		 */
>> -		if (needs_modeset(crtc_state))
>> +		if (needs_modeset(crtc_state) &&
>> +		    to_intel_plane_state(plane->state)->visible)
>>  			ret = i915_gem_object_wait_rendering(old_obj, true);
>>  
>>  		/* Swallow -EIO errors to allow updates during hw lockup. */
>> @@ -13335,11 +13378,21 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  		if (ret)
>>  			DRM_DEBUG_KMS("failed to attach phys object\n");
>>  	} else {
>> -		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
>> +		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state);
>>  	}
>>  
>> -	if (ret == 0)
>> +	if (ret == 0) {
>> +		if (obj && obj->last_write_req &&
>> +		    !i915_gem_request_completed(obj->last_write_req, true)) {
>> +			struct intel_plane_state *plane_state =
>> +				to_intel_plane_state(new_state);
>> +
>> +			i915_gem_request_assign(&plane_state->wait_req,
>> +						obj->last_write_req);
>> +		}
>> +
>>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -13357,9 +13410,12 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct intel_plane_state *old_intel_state;
>>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
>>  
>> +	old_intel_state = to_intel_plane_state(old_state);
>> +
>>  	if (!obj && !old_obj)
>>  		return;
>>  
>> @@ -13371,6 +13427,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>>  	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
>>  	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
>> +
>> +	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
>> +
>>  }
>>  
>>  int
>> @@ -15348,8 +15407,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  		mutex_lock(&dev->struct_mutex);
>>  		ret = intel_pin_and_fence_fb_obj(c->primary,
>>  						 c->primary->fb,
>> -						 c->primary->state,
>> -						 NULL, NULL);
>> +						 c->primary->state);
>>  		mutex_unlock(&dev->struct_mutex);
>>  		if (ret) {
>>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index cfdb0f2714cd..852a0d192f82 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -270,6 +270,9 @@ struct intel_plane_state {
>>  	int scaler_id;
>>  
>>  	struct drm_intel_sprite_colorkey ckey;
>> +
>> +	/* async flip related structures */
>> +	struct drm_i915_gem_request *wait_req;
>>  };
>>  
>>  struct intel_initial_plane_config {
>> @@ -1055,9 +1058,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>  				    struct drm_modeset_acquire_ctx *ctx);
>>  int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>  			       struct drm_framebuffer *fb,
>> -			       const struct drm_plane_state *plane_state,
>> -			       struct intel_engine_cs *pipelined,
>> -			       struct drm_i915_gem_request **pipelined_request);
>> +			       const struct drm_plane_state *plane_state);
>>  struct drm_framebuffer *
>>  __intel_framebuffer_create(struct drm_device *dev,
>>  			   struct drm_mode_fb_cmd2 *mode_cmd,
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 65329127f0b9..51afee61ba7e 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -155,7 +155,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>>  	}
>>  
>>  	/* Flush everything out, we'll be doing GTT only from now on */
>> -	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
>> +	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
>>  	if (ret) {
>>  		DRM_ERROR("failed to pin obj: %d\n", ret);
>>  		goto out_fb;
>> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
>> index 444542696a2c..1b18cc6bdbd6 100644
>> --- a/drivers/gpu/drm/i915/intel_overlay.c
>> +++ b/drivers/gpu/drm/i915/intel_overlay.c
>> @@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>>  	if (ret != 0)
>>  		return ret;
>>  
>> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
>> +	ret = i915_gem_object_wait_rendering(new_bo, true);
> Again, I'm not super familiar with GEM internals...can this be a
> behavior change from the previous code?  Originally the pin_to_display
> plane function would have passed (obj->base.pending_write_domain == 0)
> as the second parameter here (readonly), but you're unconditionally
> passing true.  Can there not be pending writes against this object?
I don't think it would be important in the case of overlays. But maybe I should
just replace it with a call to i915_gem_object_sync and wait for full object idle.
> Overall I feel like this patch goes a bit too far outside my area of
> expertise for me to serve as the final reviewer for it.  Maybe it would
> be good to have someone like Chris or Ville take a look since they have
> a lot more experience in this area.
>
Slightly outside my area too. :)

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

Re: [PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Chris Wilson]

On Mon, Nov 02, 2015 at 02:13:48PM +0100, Maarten Lankhorst wrote:
> Op 29-10-15 om 01:30 schreef Matt Roper:
> > On Wed, Sep 23, 2015 at 01:27:12PM +0200, Maarten Lankhorst wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> >> index 444542696a2c..1b18cc6bdbd6 100644
> >> --- a/drivers/gpu/drm/i915/intel_overlay.c
> >> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> >> @@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> >>  	if (ret != 0)
> >>  		return ret;
> >>  
> >> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
> >> +	ret = i915_gem_object_wait_rendering(new_bo, true);
> > Again, I'm not super familiar with GEM internals...can this be a
> > behavior change from the previous code?  Originally the pin_to_display
> > plane function would have passed (obj->base.pending_write_domain == 0)
> > as the second parameter here (readonly), but you're unconditionally
> > passing true.  Can there not be pending writes against this object?
> I don't think it would be important in the case of overlays. But maybe I should
> just replace it with a call to i915_gem_object_sync and wait for full object idle.

Technically it removes a call to set-cache, acquiring a GGTT offset and
pinning it, and a final set-to-gtt. 

Quite a major and broken change.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Maarten Lankhorst]

Op 02-11-15 om 14:46 schreef Chris Wilson:
> On Mon, Nov 02, 2015 at 02:13:48PM +0100, Maarten Lankhorst wrote:
>> Op 29-10-15 om 01:30 schreef Matt Roper:
>>> On Wed, Sep 23, 2015 at 01:27:12PM +0200, Maarten Lankhorst wrote:
>>>> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
>>>> index 444542696a2c..1b18cc6bdbd6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_overlay.c
>>>> +++ b/drivers/gpu/drm/i915/intel_overlay.c
>>>> @@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>>>>  	if (ret != 0)
>>>>  		return ret;
>>>>  
>>>> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
>>>> +	ret = i915_gem_object_wait_rendering(new_bo, true);
>>> Again, I'm not super familiar with GEM internals...can this be a
>>> behavior change from the previous code?  Originally the pin_to_display
>>> plane function would have passed (obj->base.pending_write_domain == 0)
>>> as the second parameter here (readonly), but you're unconditionally
>>> passing true.  Can there not be pending writes against this object?
>> I don't think it would be important in the case of overlays. But maybe I should
>> just replace it with a call to i915_gem_object_sync and wait for full object idle.
> Technically it removes a call to set-cache, acquiring a GGTT offset and
> pinning it, and a final set-to-gtt. 
>
> Quite a major and broken change.
> -Chris
>
No? pin_to_display_plane is called immediately below in this patch, it's just not shown here in the comments because it was about using wait_rendering.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] drm/i915: Change locking for struct_mutex, v2.

[Maarten Lankhorst]

Op 02-11-15 om 14:06 schreef Chris Wilson:
> On Mon, Nov 02, 2015 at 01:57:59PM +0100, Maarten Lankhorst wrote:
>> struct_mutex is being locked for every plane in intel_prepare_plane_fb and
>> intel_cleanup_plane_fb. This can be optimized by acquiring struct_mutex first
>> before calling the atomic helpers. This way the lock only needs to be acquired
>> twice in ->atomic_commit(). Once for pinning new framebuffers at the start,
>> the second time for unpinning old framebuffer.
> A little explanation that you move the locking into the caller would
> help clarify the patch.
>
>> @@ -13453,10 +13461,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>> @@ -13520,7 +13521,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> Please document that you now expect both of these functions to be called
> with struct_mutex held. Also is there any opportunity to reduce the
> struct_mutex lock time?
>
Patch 5/5 reduces locking time. It moves most waiting out of struct_mutex except for the full idle during modesets. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Wait for object idle without locks in atomic_commit.

[Chris Wilson]

On Mon, Nov 02, 2015 at 02:53:00PM +0100, Maarten Lankhorst wrote:
> Op 02-11-15 om 14:46 schreef Chris Wilson:
> > On Mon, Nov 02, 2015 at 02:13:48PM +0100, Maarten Lankhorst wrote:
> >> Op 29-10-15 om 01:30 schreef Matt Roper:
> >>> On Wed, Sep 23, 2015 at 01:27:12PM +0200, Maarten Lankhorst wrote:
> >>>> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> >>>> index 444542696a2c..1b18cc6bdbd6 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_overlay.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> >>>> @@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> >>>>  	if (ret != 0)
> >>>>  		return ret;
> >>>>  
> >>>> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL,
> >>>> +	ret = i915_gem_object_wait_rendering(new_bo, true);
> >>> Again, I'm not super familiar with GEM internals...can this be a
> >>> behavior change from the previous code?  Originally the pin_to_display
> >>> plane function would have passed (obj->base.pending_write_domain == 0)
> >>> as the second parameter here (readonly), but you're unconditionally
> >>> passing true.  Can there not be pending writes against this object?
> >> I don't think it would be important in the case of overlays. But maybe I should
> >> just replace it with a call to i915_gem_object_sync and wait for full object idle.
> > Technically it removes a call to set-cache, acquiring a GGTT offset and
> > pinning it, and a final set-to-gtt. 
> >
> > Quite a major and broken change.
> > -Chris
> >
> No? pin_to_display_plane is called immediately below in this patch, it's just not shown here in the comments because it was about using wait_rendering.

Bah.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx