drm/i915: Interruptible framebuffer pinning.

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 18 insertions(+), 30 deletions(-)

drivers/gpu/drm/i915/intel_display.c +10 -27

index fc0086748b71..ac97af69be62 100644
@@ -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);

Quoted 95 lines

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 18 insertions(+), 30 deletions(-)

drivers/gpu/drm/i915/intel_display.c +10 -27

index fc0086748b71..ac97af69be62 100644
@@ -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);

drivers/gpu/drm/i915/intel_display.c intel_cleanup_plane_fb(struct drm_plane *plane, · line 13357

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +7 -2

+	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 <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -5

index ac97af69be62..25e1eac260fd 100644
@@ -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;
 }

Quoted 7 lines · 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.

Quoted 4 lines

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

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

Quoted 40 lines

---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -5

index ac97af69be62..25e1eac260fd 100644
@@ -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;
 }

drivers/gpu/drm/i915/intel_display.c intel_prepare_plane_fb(struct drm_plane *plane, · line 13310

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +3 -1

@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index f1975f267710..25a891aa3824 100644
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just do

the

drivers/gpu/drm/i915/intel_atomic.c +0 -2

 				 * minimum required validation.
 				 */
-				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-					intel_crtc->atomic.wait_for_flips =

true;

drivers/gpu/drm/i915/intel_atomic.c +0 -0

 				crtc_state->base.planes_changed = true;
 			}

drivers/gpu/drm/i915/intel_display.c +12 -30

index 25e1eac260fd..cd651ff6c15b 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 		spin_lock_irq(&dev->event_lock);

drivers/gpu/drm/i915/intel_display.c +1 -0

+	return 0;
 }
 

/* Program iCLKIP clock to the desired frequency */

drivers/gpu/drm/i915/intel_display.c +36 -10

@@ -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.

drivers/gpu/drm/i915/intel_display.c +2 -0

+	}
+
 	if (!obj) {
 		ret = 0;

drivers/gpu/drm/i915/intel_display.c +1 -1

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

drivers/gpu/drm/i915/intel_drv.h +0 -1

index a2f0b981e26c..cfdb0f2714cd 100644
@@ -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;

Quoted 24 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_display.c +19 -0

@@ -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. */

drivers/gpu/drm/i915/intel_display.c intel_prepare_plane_fb(struct drm_plane *plane, · line 13322

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+		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.

Quoted 38 lines · Ander Conselvan De Oliveira

Quoted 28 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_display.c +13 -1

@@ -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

drivers/gpu/drm/i915/intel_display.c +4 -0

+		 * can safely continue.
+		 */
+		if (needs_modeset(crtc_state))
+			ret = i915_gem_object_wait_rendering(old_obj,

true);

(quoted diff) +2 -0

+
+		/* Swallow -EIO errors to allow updates during hw lockup.

*/

drivers/gpu/drm/i915/intel_display.c intel_prepare_plane_fb(struct drm_plane *plane, · line 13322

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+		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

Quoted 2 lines

-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

Quoted 39 lines · Daniel Vetter

Quoted 38 lines · Ander Conselvan De Oliveira

Quoted 28 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_display.c +13 -1

@@ -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

drivers/gpu/drm/i915/intel_display.c +4 -0

+		 * can safely continue.
+		 */
+		if (needs_modeset(crtc_state))
+			ret = i915_gem_object_wait_rendering(old_obj,

true);

(quoted diff) +2 -0

+
+		/* Swallow -EIO errors to allow updates during hw lockup.

*/

drivers/gpu/drm/i915/intel_display.c intel_prepare_plane_fb(struct drm_plane *plane, · line 13322

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+		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

Quoted 8 lines

Quoted 2 lines

-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.

Quoted 59 lines · Ander Conselvan De Oliveira

Quoted 49 lines · Daniel Vetter

Quoted 47 lines · Ander Conselvan De Oliveira

Quoted 37 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_display.c +5 -1

@@ -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

(quoted diff) +1 -0

+		 * MI_WAIT_FOR_EVENT inside a user batch buffer on

the

(quoted diff) +1 -0

+		 * current scanout is retired before unpinning the

old

(quoted diff) +1 -0

+		 * framebuffer. Note that we rely on userspace

rendering

(quoted diff) +1 -0

+		 * into the buffer attached to the pipe they are

waiting

(quoted diff) +1 -0

+		 * on. If not, userspace generates a GPU hang with

IPEHR

drivers/gpu/drm/i915/intel_display.c +3 -0

+		 * point to the MI_WAIT_FOR_EVENT.
+		 *
+		 * This should only fail upon a hung GPU, in which

case
we

drivers/gpu/drm/i915/intel_display.c +4 -0

+		 * can safely continue.
+		 */
+		if (needs_modeset(crtc_state))
+			ret = i915_gem_object_wait_rendering(old_obj,

true);

(quoted diff) +2 -0

+
+		/* Swallow -EIO errors to allow updates during hw

lockup.
*/

drivers/gpu/drm/i915/intel_display.c intel_prepare_plane_fb(struct drm_plane *plane, · line 13322

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+		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

Quoted 8 lines

Quoted 2 lines

-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.

Quoted 5 lines

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.

Quoted 96 lines

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index f1975f267710..25a891aa3824 100644
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just do

the

drivers/gpu/drm/i915/intel_atomic.c +0 -2

 				 * minimum required validation.
 				 */
-				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-					intel_crtc->atomic.wait_for_flips =

true;

drivers/gpu/drm/i915/intel_atomic.c +0 -0

 				crtc_state->base.planes_changed = true;
 			}

drivers/gpu/drm/i915/intel_display.c +12 -30

index 25e1eac260fd..cd651ff6c15b 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 		spin_lock_irq(&dev->event_lock);

drivers/gpu/drm/i915/intel_display.c void intel_crtc_wait_for_pending_flips(struct drm_crtc · line 3894

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +0 -5

@@ -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?

Quoted 96 lines

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index f1975f267710..25a891aa3824 100644
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just do

the

drivers/gpu/drm/i915/intel_atomic.c +0 -2

 				 * minimum required validation.
 				 */
-				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-					intel_crtc->atomic.wait_for_flips =

true;

drivers/gpu/drm/i915/intel_atomic.c +0 -0

 				crtc_state->base.planes_changed = true;
 			}

drivers/gpu/drm/i915/intel_display.c +12 -30

index 25e1eac260fd..cd651ff6c15b 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 		spin_lock_irq(&dev->event_lock);

drivers/gpu/drm/i915/intel_display.c void intel_crtc_wait_for_pending_flips(struct drm_crtc · line 3894

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +0 -5

@@ -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?

Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:

Quoted 120 lines · Maarten Lankhorst

Quoted 104 lines

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++----------

----

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index f1975f267710..25a891aa3824 100644
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just

do
the

drivers/gpu/drm/i915/intel_atomic.c +0 -1

 				 * minimum required validation.
 				 */
-				if (plane->type ==

DRM_PLANE_TYPE_PRIMARY)

drivers/gpu/drm/i915/intel_atomic.c +0 -1

-					intel_crtc->atomic.wait_for_flips

=
true;

drivers/gpu/drm/i915/intel_atomic.c +0 -0

 				crtc_state->base.planes_changed = true;
 			}

drivers/gpu/drm/i915/intel_display.c +12 -30

index 25e1eac260fd..cd651ff6c15b 100644
@@ -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) {

drivers/gpu/drm/i915/intel_display.c +0 -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)

drivers/gpu/drm/i915/intel_display.c

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c +0 -5

 		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.

Quoted 3 lines

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 <[email protected]>

--

drivers/gpu/drm/i915/intel_display.c +0 -0

index f59029c3e577..95c59b5202c5 100644

Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:

Quoted 111 lines · Maarten Lankhorst

Quoted 96 lines

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 -
 drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 3 files changed, 62 insertions(+), 49 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index f1975f267710..25a891aa3824 100644
@@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 				 * but since this plane is unchanged just do

the

drivers/gpu/drm/i915/intel_atomic.c +0 -2

 				 * minimum required validation.
 				 */
-				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-					intel_crtc->atomic.wait_for_flips =

true;

drivers/gpu/drm/i915/intel_atomic.c +0 -0

 				crtc_state->base.planes_changed = true;
 			}

drivers/gpu/drm/i915/intel_display.c +12 -30

index 25e1eac260fd..cd651ff6c15b 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 		spin_lock_irq(&dev->event_lock);

drivers/gpu/drm/i915/intel_display.c void intel_crtc_wait_for_pending_flips(struct drm_crtc · line 3894

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +0 -5

@@ -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.

Quoted 3 lines

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 <[email protected]>

--

drivers/gpu/drm/i915/intel_display.c +2 -1

index f59029c3e577..95c59b5202c5 100644
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Only acquire the struct_mutex once, and interruptibly.

Signed-off-by Maarten Lankhorst <[email protected]>

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

drivers/gpu/drm/i915/intel_display.c +9 -10

index cd651ff6c15b..2f046134cc9a 100644
@@ -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

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.

Quoted 5 lines

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.

Review intel_cleanup_plane_fb(struct drm_plane *plane,

Quoted 2 lines

(quoted diff) +0 -0

@@ -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?

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 3bf8a9b771d0..ec72fd457499 100644
@@ -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);

drivers/gpu/drm/i915/i915_gem.c +0 -6

index 46f0e83ee6ee..ab02182c47a5 100644
@@ -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.
 	 */

drivers/gpu/drm/i915/intel_atomic_plane.c +2 -0

index a11980696595..c6bb0fc1edfb 100644
@@ -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);
 }
 

drivers/gpu/drm/i915/intel_display.c +1 -3

index 2f046134cc9a..d817c44ee428 100644
@@ -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;

drivers/gpu/drm/i915/intel_display.c +0 -0

 	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.
 	 */

drivers/gpu/drm/i915/intel_display.c +62 -7

 	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",

drivers/gpu/drm/i915/intel_drv.h +4 -3

index cfdb0f2714cd..852a0d192f82 100644
@@ -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,

drivers/gpu/drm/i915/intel_fbdev.c +1 -1

index 65329127f0b9..51afee61ba7e 100644
@@ -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;

drivers/gpu/drm/i915/intel_overlay.c +0 -0

index 444542696a2c..1b18cc6bdbd6 100644

drivers/gpu/drm/i915/intel_overlay.c +4 -0

+	if (ret)
+		return ret;
+
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0,
 						   &i915_ggtt_view_normal);
 	if (ret != 0)
 		return ret;

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 88 lines

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 3bf8a9b771d0..ec72fd457499 100644
@@ -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);

drivers/gpu/drm/i915/i915_gem.c +0 -6

index 46f0e83ee6ee..ab02182c47a5 100644
@@ -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.
 	 */

drivers/gpu/drm/i915/intel_atomic_plane.c +2 -0

index a11980696595..c6bb0fc1edfb 100644
@@ -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);
 }
 

drivers/gpu/drm/i915/intel_display.c +1 -3

index 2f046134cc9a..d817c44ee428 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_plane *plane, · line 2351

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +2 -2

-	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?

Quoted 16 lines

drivers/gpu/drm/i915/intel_display.c +6 -1

 	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,

drivers/gpu/drm/i915/intel_display.c static int intel_crtc_page_flip(struct drm_crtc *crtc, · line 11365

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-					 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?

Quoted 195 lines

drivers/gpu/drm/i915/intel_display.c +62 -7

 	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",

drivers/gpu/drm/i915/intel_drv.h +4 -3

index cfdb0f2714cd..852a0d192f82 100644
@@ -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,

drivers/gpu/drm/i915/intel_fbdev.c +1 -1

index 65329127f0b9..51afee61ba7e 100644
@@ -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;

drivers/gpu/drm/i915/intel_overlay.c +0 -0

index 444542696a2c..1b18cc6bdbd6 100644
@@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 

drivers/gpu/drm/i915/intel_overlay.c static int intel_overlay_do_put_image(struct intel_overlay *overlay, · line 752

Quoted 2 lines

drivers/gpu/drm/i915/intel_overlay.c +1 -1

-	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.

Op 29-10-15 om 01:30 schreef Matt Roper:

Quoted 9 lines · Maarten Lankhorst

Quoted 8 lines

drivers/gpu/drm/i915/intel_overlay.c +0 -0

index 444542696a2c..1b18cc6bdbd6 100644
@@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_overlay.c static int intel_overlay_do_put_image(struct intel_overlay *overlay, · line 752

Quoted 2 lines

drivers/gpu/drm/i915/intel_overlay.c +1 -1

-	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.

Quoted 17 lines

Op 29-10-15 om 01:30 schreef Matt Roper:

Quoted 16 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_overlay.c static int intel_overlay_do_put_image(struct intel_overlay *overlay, · line 752

Quoted 10 lines

drivers/gpu/drm/i915/intel_overlay.c +1 -1

index 444542696a2c..1b18cc6bdbd6 100644
@@ -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?

Quoted 2 lines

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

Op 02-11-15 om 14:46 schreef Chris Wilson:

Quoted 20 lines · Maarten Lankhorst

Quoted 19 lines

Op 29-10-15 om 01:30 schreef Matt Roper:

Quoted 9 lines · Maarten Lankhorst

Quoted 8 lines

drivers/gpu/drm/i915/intel_overlay.c +0 -0

index 444542696a2c..1b18cc6bdbd6 100644
@@ -749,7 +749,11 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_overlay.c static int intel_overlay_do_put_image(struct intel_overlay *overlay, · line 752

Quoted 2 lines

drivers/gpu/drm/i915/intel_overlay.c +1 -1

-	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.

Quoted 6 lines

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.