[PATCH 0/5] drm/i915: Make waiting interruptible.

[PATCH 0/5] drm/i915: Make waiting interruptible.

[Maarten Lankhorst]

Now that i915 is fully atomic it's time to make the waits interruptible.

This can be done by making sure prepare_plane_fb is always called,
because it's the logical place to put such waits in.

This also cleans up the disabled_planes hack, the disabling
planes case is now handled by prepare_plane_fb correctly.

Maarten Lankhorst (5):
  drm/i915: Handle return value in intel_pin_and_fence_fb_obj.
  drm/atomic: Make prepare_fb/cleanup_fb only take state.
  drm/i915: Make prepare_plane_fb fully interruptible.
  drm/i915: Remove wait_for_pending_flips from disable_noatomic.
  drm/i915: Make wait_for_flips interruptible.

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |   4 +-
 drivers/gpu/drm/drm_atomic_helper.c             |  21 +---
 drivers/gpu/drm/drm_plane_helper.c              |   6 +-
 drivers/gpu/drm/i915/intel_atomic.c             |   2 -
 drivers/gpu/drm/i915/intel_display.c            | 144 +++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h                |   5 -
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c       |  10 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c       |   9 +-
 drivers/gpu/drm/omapdrm/omap_plane.c            |  10 +-
 drivers/gpu/drm/sti/sti_drm_plane.c             |   2 -
 drivers/gpu/drm/tegra/dc.c                      |   2 -
 include/drm/drm_plane_helper.h                  |   2 -
 12 files changed, 97 insertions(+), 120 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: Handle return value in intel_pin_and_fence_fb_obj.

[Maarten Lankhorst]

-EDEADLK has special meaning in atomic, but get_fence may call
i915_find_fence_reg which can return -EDEADLK.

This has special meaning in the atomic world, so convert the error
to -EBUSY for this case.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ede652867596..786018eaf393 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2395,8 +2395,11 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * a fence as the cost is not that onerous.
 	 */
 	ret = i915_gem_object_get_fence(obj);
-	if (ret)
+	if (ret) {
+		if (ret == -EDEADLK)
+			ret = -EBUSY;
 		goto err_unpin;
+	}
 
 	i915_gem_object_pin_fence(obj);
 
-- 
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: Handle return value in intel_pin_and_fence_fb_obj.

[Chris Wilson]

On Thu, Jul 16, 2015 at 02:57:47PM +0200, Maarten Lankhorst wrote:
> -EDEADLK has special meaning in atomic, but get_fence may call
> i915_find_fence_reg which can return -EDEADLK.
> 
> This has special meaning in the atomic world, so convert the error
> to -EBUSY for this case.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ede652867596..786018eaf393 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2395,8 +2395,11 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * a fence as the cost is not that onerous.
>  	 */
>  	ret = i915_gem_object_get_fence(obj);
> -	if (ret)
> +	if (ret) {

/* A quick explanation here would be delightful */

> +		if (ret == -EDEADLK)
> +			ret = -EBUSY;
>  		goto err_unpin;
-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

[PATCH v1.1 1/5] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Maarten Lankhorst]

-EDEADLK has special meaning in atomic, but get_fence may call
i915_find_fence_reg which can return -EDEADLK.

This has special meaning in the atomic world, so convert the error
to -EBUSY for this case.

Changes since v1:
- Add comment in the code.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Like this?

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfee4771..11387f5ed681 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2395,8 +2395,20 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * a fence as the cost is not that onerous.
 	 */
 	ret = i915_gem_object_get_fence(obj);
-	if (ret)
+	if (ret) {
+		if (ret == -EDEADLK) {
+			/*
+			 * -EDEADLK means there are no free fences
+			 * and no pending flips.
+			 *
+			 * This is propagated to atomic, but it uses
+			 * -EDEADLK to force a locking recovery, so
+			 * change the returned error to -EBUSY.
+			 */
+			ret = -EBUSY;
+		}
 		goto err_unpin;
+	}
 
 	i915_gem_object_pin_fence(obj);
 

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

Re: [PATCH v1.1 1/5] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Ander Conselvan De Oliveira]

On Tue, 2015-07-21 at 16:09 +0200, Maarten Lankhorst wrote:
> -EDEADLK has special meaning in atomic, but get_fence may call
> i915_find_fence_reg which can return -EDEADLK.
> 
> This has special meaning in the atomic world, so convert the error
> to -EBUSY for this case.

Doesn't this change the behavior of intel_crtc_page_flip() slightly? It
now would return -EBUSY to user space if it can't find a fence instead
of -EDEADLK. Not sure if that is a problem though. I don't expect user
space would actually check for -EDEADLK.

Ander

> Changes since v1:
> - Add comment in the code.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Like this?
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfee4771..11387f5ed681 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2395,8 +2395,20 @@ intel_pin_and_fence_fb_obj(struct drm_plane 
> *plane,
>  	 * a fence as the cost is not that onerous.
>  	 */
>  	ret = i915_gem_object_get_fence(obj);
> -	if (ret)
> +	if (ret) {
> +		if (ret == -EDEADLK) {
> +			/*
> +			 * -EDEADLK means there are no free fences
> +			 * and no pending flips.
> +			 *
> +			 * This is propagated to atomic, but it uses
> +			 * -EDEADLK to force a locking recovery, so
> +			 * change the returned error to -EBUSY.
> +			 */
> +			ret = -EBUSY;
> +		}
>  		goto err_unpin;
> +	}
>  
>  	i915_gem_object_pin_fence(obj);
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1.1 1/5] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Chris Wilson]

On Fri, Jul 24, 2015 at 04:26:27PM +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-07-21 at 16:09 +0200, Maarten Lankhorst wrote:
> > -EDEADLK has special meaning in atomic, but get_fence may call
> > i915_find_fence_reg which can return -EDEADLK.
> > 
> > This has special meaning in the atomic world, so convert the error
> > to -EBUSY for this case.
> 
> Doesn't this change the behavior of intel_crtc_page_flip() slightly? It
> now would return -EBUSY to user space if it can't find a fence instead
> of -EDEADLK. Not sure if that is a problem though. I don't expect user
> space would actually check for -EDEADLK.

It's extreme, but we could fix the -EDEADLK by dropping the struct_mutex
and flushing the unpin workqueue (there is no other conceivable way of
triggering EDEADLK here other than uncompleted flips except for a kernel
bug ofc). I can confidently state that no one has or ever will see an
EDEADLK here - and even if it did userspace is best to treat it exactly
the same as any other error path, cancel the flip and try again later.
-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 v1.1 1/5] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Maarten Lankhorst]

Op 24-07-15 om 15:26 schreef Ander Conselvan De Oliveira:
> On Tue, 2015-07-21 at 16:09 +0200, Maarten Lankhorst wrote:
>> -EDEADLK has special meaning in atomic, but get_fence may call
>> i915_find_fence_reg which can return -EDEADLK.
>>
>> This has special meaning in the atomic world, so convert the error
>> to -EBUSY for this case.
> Doesn't this change the behavior of intel_crtc_page_flip() slightly? It
> now would return -EBUSY to user space if it can't find a fence instead
> of -EDEADLK. Not sure if that is a problem though. I don't expect user
> space would actually check for -EDEADLK.
>
It would, but if we ever make page_flip atomic we will have to do something like this anyway..

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

[PATCH 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state.

[Maarten Lankhorst]

This removes the need to separately track fb changes i915.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  4 +-
 drivers/gpu/drm/drm_atomic_helper.c             | 21 +++-------
 drivers/gpu/drm/drm_plane_helper.c              |  6 +--
 drivers/gpu/drm/i915/intel_display.c            | 52 ++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h                |  3 --
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c       | 10 ++++-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c       |  9 ++++-
 drivers/gpu/drm/omapdrm/omap_plane.c            | 10 +++--
 drivers/gpu/drm/sti/sti_drm_plane.c             |  2 -
 drivers/gpu/drm/tegra/dc.c                      |  2 -
 include/drm/drm_plane_helper.h                  |  2 -
 11 files changed, 54 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index be9fa8220499..36fda86b3518 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -712,11 +712,13 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p,
 }
 
 static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p,
-					struct drm_framebuffer *fb,
 					const struct drm_plane_state *new_state)
 {
 	struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
 
+	if (!new_state->fb)
+		return 0;
+
 	return atmel_hlcdc_layer_update_start(&plane->layer);
 }
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0898afbc9e23..e52dfc828e60 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1063,17 +1063,14 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		const struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = state->planes[i];
 		struct drm_plane_state *plane_state = state->plane_states[i];
-		struct drm_framebuffer *fb;
 
 		if (!plane)
 			continue;
 
 		funcs = plane->helper_private;
 
-		fb = plane_state->fb;
-
-		if (fb && funcs->prepare_fb) {
-			ret = funcs->prepare_fb(plane, fb, plane_state);
+		if (funcs->prepare_fb) {
+			ret = funcs->prepare_fb(plane, plane_state);
 			if (ret)
 				goto fail;
 		}
@@ -1086,17 +1083,14 @@ fail:
 		const struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = state->planes[i];
 		struct drm_plane_state *plane_state = state->plane_states[i];
-		struct drm_framebuffer *fb;
 
 		if (!plane)
 			continue;
 
 		funcs = plane->helper_private;
 
-		fb = state->plane_states[i]->fb;
-
-		if (fb && funcs->cleanup_fb)
-			funcs->cleanup_fb(plane, fb, plane_state);
+		if (funcs->cleanup_fb)
+			funcs->cleanup_fb(plane, plane_state);
 
 	}
 
@@ -1252,14 +1246,11 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
-		struct drm_framebuffer *old_fb;
 
 		funcs = plane->helper_private;
 
-		old_fb = plane_state->fb;
-
-		if (old_fb && funcs->cleanup_fb)
-			funcs->cleanup_fb(plane, old_fb, plane_state);
+		if (funcs->cleanup_fb)
+			funcs->cleanup_fb(plane, plane_state);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index b07a213f5655..03b7afe455e6 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -425,7 +425,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 
 	if (plane_funcs->prepare_fb && plane_state->fb &&
 	    plane_state->fb != old_fb) {
-		ret = plane_funcs->prepare_fb(plane, plane_state->fb,
+		ret = plane_funcs->prepare_fb(plane,
 					      plane_state);
 		if (ret)
 			goto out;
@@ -478,8 +478,8 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 		ret = 0;
 	}
 
-	if (plane_funcs->cleanup_fb && old_fb)
-		plane_funcs->cleanup_fb(plane, old_fb, plane_state);
+	if (plane_funcs->cleanup_fb)
+		plane_funcs->cleanup_fb(plane, plane_state);
 out:
 	if (plane_state) {
 		if (plane->funcs->atomic_destroy_state)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 786018eaf393..3f82214b737b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4775,17 +4775,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);
@@ -11668,14 +11657,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;
 
@@ -13488,21 +13469,23 @@ static void intel_shared_dpll_init(struct drm_device *dev)
  */
 int
 intel_prepare_plane_fb(struct drm_plane *plane,
-		       struct drm_framebuffer *fb,
 		       const struct drm_plane_state *new_state)
 {
 	struct drm_device *dev = plane->dev;
+	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);
@@ -13529,21 +13512,26 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  */
 void
 intel_cleanup_plane_fb(struct drm_plane *plane,
-		       struct drm_framebuffer *fb,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_gem_object *obj = intel_fb_obj(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->fb);
 
-	if (WARN_ON(!obj))
+	if (old_obj == obj)
 		return;
 
-	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev)->cursor_needs_physical) {
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(fb, old_state);
-		mutex_unlock(&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);
+
+	/* 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 0fcfa7f179c4..e6b04941f9ca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -495,7 +495,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;
@@ -1037,10 +1036,8 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
 int intel_prepare_plane_fb(struct drm_plane *plane,
-			   struct drm_framebuffer *fb,
 			   const struct drm_plane_state *new_state);
 void intel_cleanup_plane_fb(struct drm_plane *plane,
-			    struct drm_framebuffer *fb,
 			    const struct drm_plane_state *old_state);
 int intel_plane_atomic_get_property(struct drm_plane *plane,
 				    const struct drm_plane_state *state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index 0d1dbb737933..60e83c3765c2 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -98,22 +98,28 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
 };
 
 static int mdp4_plane_prepare_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *new_state)
 {
 	struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
 	struct mdp4_kms *mdp4_kms = get_kms(plane);
+	struct drm_framebuffer *fb = new_state->fb;
+
+	if (!fb)
+		return 0;
 
 	DBG("%s: prepare: FB[%u]", mdp4_plane->name, fb->base.id);
 	return msm_framebuffer_prepare(fb, mdp4_kms->id);
 }
 
 static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *old_state)
 {
 	struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
 	struct mdp4_kms *mdp4_kms = get_kms(plane);
+	struct drm_framebuffer *fb = old_state->fb;
+
+	if (!fb)
+		return;
 
 	DBG("%s: cleanup: FB[%u]", mdp4_plane->name, fb->base.id);
 	msm_framebuffer_cleanup(fb, mdp4_kms->id);
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 57b8f56ae9d0..d0f627b962f3 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -156,11 +156,14 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
 };
 
 static int mdp5_plane_prepare_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *new_state)
 {
 	struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
 	struct mdp5_kms *mdp5_kms = get_kms(plane);
+	struct drm_framebuffer *fb = new_state->fb;
+
+	if (!new_state->fb)
+		return 0;
 
 	DBG("%s: prepare: FB[%u]", mdp5_plane->name, fb->base.id);
 	return msm_framebuffer_prepare(fb, mdp5_kms->id);
@@ -172,6 +175,10 @@ static void mdp5_plane_cleanup_fb(struct drm_plane *plane,
 {
 	struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
 	struct mdp5_kms *mdp5_kms = get_kms(plane);
+	struct drm_framebuffer *fb = old_state->fb;
+
+	if (!fb)
+		return;
 
 	DBG("%s: cleanup: FB[%u]", mdp5_plane->name, fb->base.id);
 	msm_framebuffer_cleanup(fb, mdp5_kms->id);
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 098904696a5c..09e363bb55f2 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -60,17 +60,19 @@ to_omap_plane_state(struct drm_plane_state *state)
 }
 
 static int omap_plane_prepare_fb(struct drm_plane *plane,
-				 struct drm_framebuffer *fb,
 				 const struct drm_plane_state *new_state)
 {
-	return omap_framebuffer_pin(fb);
+	if (!new_state->fb)
+		return 0;
+
+	return omap_framebuffer_pin(new_state->fb);
 }
 
 static void omap_plane_cleanup_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *old_state)
 {
-	omap_framebuffer_unpin(fb);
+	if (old_state->fb)
+		omap_framebuffer_unpin(old_state->fb);
 }
 
 static void omap_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/sti/sti_drm_plane.c b/drivers/gpu/drm/sti/sti_drm_plane.c
index 64d4ed43dda3..f3717292e748 100644
--- a/drivers/gpu/drm/sti/sti_drm_plane.c
+++ b/drivers/gpu/drm/sti/sti_drm_plane.c
@@ -147,14 +147,12 @@ static struct drm_plane_funcs sti_drm_plane_funcs = {
 };
 
 static int sti_drm_plane_prepare_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *new_state)
 {
 	return 0;
 }
 
 static void sti_drm_plane_cleanup_fb(struct drm_plane *plane,
-				   struct drm_framebuffer *fb,
 				   const struct drm_plane_state *old_fb)
 {
 }
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a287e4fec865..d447701173e6 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -472,14 +472,12 @@ static const struct drm_plane_funcs tegra_primary_plane_funcs = {
 };
 
 static int tegra_plane_prepare_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *new_state)
 {
 	return 0;
 }
 
 static void tegra_plane_cleanup_fb(struct drm_plane *plane,
-				   struct drm_framebuffer *fb,
 				   const struct drm_plane_state *old_fb)
 {
 }
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 96e16283afb9..5ffca4e51e40 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -59,10 +59,8 @@ extern int drm_crtc_init(struct drm_device *dev,
  */
 struct drm_plane_helper_funcs {
 	int (*prepare_fb)(struct drm_plane *plane,
-			  struct drm_framebuffer *fb,
 			  const struct drm_plane_state *new_state);
 	void (*cleanup_fb)(struct drm_plane *plane,
-			   struct drm_framebuffer *fb,
 			   const struct drm_plane_state *old_state);
 
 	int (*atomic_check)(struct drm_plane *plane,
-- 
2.1.0

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

[PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Maarten Lankhorst]

This removes the need to separately track fb changes i915.
    
Changes since v1:
- Add dri-devel to cc.
- Fix a check in intel's prepare and cleanup fb to take rotation
  into account.
    
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index be9fa8220499..36fda86b3518 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -712,11 +712,13 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p,
 }
 
 static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p,
-					struct drm_framebuffer *fb,
 					const struct drm_plane_state *new_state)
 {
 	struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
 
+	if (!new_state->fb)
+		return 0;
+
 	return atmel_hlcdc_layer_update_start(&plane->layer);
 }
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0898afbc9e23..e52dfc828e60 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1063,17 +1063,14 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		const struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = state->planes[i];
 		struct drm_plane_state *plane_state = state->plane_states[i];
-		struct drm_framebuffer *fb;
 
 		if (!plane)
 			continue;
 
 		funcs = plane->helper_private;
 
-		fb = plane_state->fb;
-
-		if (fb && funcs->prepare_fb) {
-			ret = funcs->prepare_fb(plane, fb, plane_state);
+		if (funcs->prepare_fb) {
+			ret = funcs->prepare_fb(plane, plane_state);
 			if (ret)
 				goto fail;
 		}
@@ -1086,17 +1083,14 @@ fail:
 		const struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = state->planes[i];
 		struct drm_plane_state *plane_state = state->plane_states[i];
-		struct drm_framebuffer *fb;
 
 		if (!plane)
 			continue;
 
 		funcs = plane->helper_private;
 
-		fb = state->plane_states[i]->fb;
-
-		if (fb && funcs->cleanup_fb)
-			funcs->cleanup_fb(plane, fb, plane_state);
+		if (funcs->cleanup_fb)
+			funcs->cleanup_fb(plane, plane_state);
 
 	}
 
@@ -1252,14 +1246,11 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
-		struct drm_framebuffer *old_fb;
 
 		funcs = plane->helper_private;
 
-		old_fb = plane_state->fb;
-
-		if (old_fb && funcs->cleanup_fb)
-			funcs->cleanup_fb(plane, old_fb, plane_state);
+		if (funcs->cleanup_fb)
+			funcs->cleanup_fb(plane, plane_state);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index b07a213f5655..03b7afe455e6 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -425,7 +425,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 
 	if (plane_funcs->prepare_fb && plane_state->fb &&
 	    plane_state->fb != old_fb) {
-		ret = plane_funcs->prepare_fb(plane, plane_state->fb,
+		ret = plane_funcs->prepare_fb(plane,
 					      plane_state);
 		if (ret)
 			goto out;
@@ -478,8 +478,8 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 		ret = 0;
 	}
 
-	if (plane_funcs->cleanup_fb && old_fb)
-		plane_funcs->cleanup_fb(plane, old_fb, plane_state);
+	if (plane_funcs->cleanup_fb)
+		plane_funcs->cleanup_fb(plane, plane_state);
 out:
 	if (plane_state) {
 		if (plane->funcs->atomic_destroy_state)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7dbfeacf0f38..b3990264e137 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4775,17 +4775,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);
@@ -11668,14 +11657,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;
 
@@ -13479,21 +13460,26 @@ static void intel_shared_dpll_init(struct drm_device *dev)
  */
 int
 intel_prepare_plane_fb(struct drm_plane *plane,
-		       struct drm_framebuffer *fb,
 		       const struct drm_plane_state *new_state)
 {
 	struct drm_device *dev = plane->dev;
+	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;
+
+	if (obj == old_obj && new_state->rotation == plane->state->rotation)
 		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);
@@ -13520,21 +13506,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  */
 void
 intel_cleanup_plane_fb(struct drm_plane *plane,
-		       struct drm_framebuffer *fb,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_gem_object *obj = intel_fb_obj(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->fb);
 
-	if (WARN_ON(!obj))
+	if (!obj && !old_obj)
 		return;
 
-	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev)->cursor_needs_physical) {
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(fb, old_state);
-		mutex_unlock(&dev->struct_mutex);
-	}
+	if (obj == old_obj && old_state->rotation == plane->state->rotation)
+		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);
+
+	/* 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 3b00d00c0bc0..bee8a9669482 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -497,7 +497,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;
@@ -1039,10 +1038,8 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
 int intel_prepare_plane_fb(struct drm_plane *plane,
-			   struct drm_framebuffer *fb,
 			   const struct drm_plane_state *new_state);
 void intel_cleanup_plane_fb(struct drm_plane *plane,
-			    struct drm_framebuffer *fb,
 			    const struct drm_plane_state *old_state);
 int intel_plane_atomic_get_property(struct drm_plane *plane,
 				    const struct drm_plane_state *state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index 0d1dbb737933..60e83c3765c2 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -98,22 +98,28 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
 };
 
 static int mdp4_plane_prepare_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *new_state)
 {
 	struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
 	struct mdp4_kms *mdp4_kms = get_kms(plane);
+	struct drm_framebuffer *fb = new_state->fb;
+
+	if (!fb)
+		return 0;
 
 	DBG("%s: prepare: FB[%u]", mdp4_plane->name, fb->base.id);
 	return msm_framebuffer_prepare(fb, mdp4_kms->id);
 }
 
 static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *old_state)
 {
 	struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
 	struct mdp4_kms *mdp4_kms = get_kms(plane);
+	struct drm_framebuffer *fb = old_state->fb;
+
+	if (!fb)
+		return;
 
 	DBG("%s: cleanup: FB[%u]", mdp4_plane->name, fb->base.id);
 	msm_framebuffer_cleanup(fb, mdp4_kms->id);
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 57b8f56ae9d0..d0f627b962f3 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -156,11 +156,14 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
 };
 
 static int mdp5_plane_prepare_fb(struct drm_plane *plane,
-		struct drm_framebuffer *fb,
 		const struct drm_plane_state *new_state)
 {
 	struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
 	struct mdp5_kms *mdp5_kms = get_kms(plane);
+	struct drm_framebuffer *fb = new_state->fb;
+
+	if (!new_state->fb)
+		return 0;
 
 	DBG("%s: prepare: FB[%u]", mdp5_plane->name, fb->base.id);
 	return msm_framebuffer_prepare(fb, mdp5_kms->id);
@@ -172,6 +175,10 @@ static void mdp5_plane_cleanup_fb(struct drm_plane *plane,
 {
 	struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
 	struct mdp5_kms *mdp5_kms = get_kms(plane);
+	struct drm_framebuffer *fb = old_state->fb;
+
+	if (!fb)
+		return;
 
 	DBG("%s: cleanup: FB[%u]", mdp5_plane->name, fb->base.id);
 	msm_framebuffer_cleanup(fb, mdp5_kms->id);
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 098904696a5c..09e363bb55f2 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -60,17 +60,19 @@ to_omap_plane_state(struct drm_plane_state *state)
 }
 
 static int omap_plane_prepare_fb(struct drm_plane *plane,
-				 struct drm_framebuffer *fb,
 				 const struct drm_plane_state *new_state)
 {
-	return omap_framebuffer_pin(fb);
+	if (!new_state->fb)
+		return 0;
+
+	return omap_framebuffer_pin(new_state->fb);
 }
 
 static void omap_plane_cleanup_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *old_state)
 {
-	omap_framebuffer_unpin(fb);
+	if (old_state->fb)
+		omap_framebuffer_unpin(old_state->fb);
 }
 
 static void omap_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/sti/sti_drm_plane.c b/drivers/gpu/drm/sti/sti_drm_plane.c
index 64d4ed43dda3..f3717292e748 100644
--- a/drivers/gpu/drm/sti/sti_drm_plane.c
+++ b/drivers/gpu/drm/sti/sti_drm_plane.c
@@ -147,14 +147,12 @@ static struct drm_plane_funcs sti_drm_plane_funcs = {
 };
 
 static int sti_drm_plane_prepare_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *new_state)
 {
 	return 0;
 }
 
 static void sti_drm_plane_cleanup_fb(struct drm_plane *plane,
-				   struct drm_framebuffer *fb,
 				   const struct drm_plane_state *old_fb)
 {
 }
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a287e4fec865..d447701173e6 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -472,14 +472,12 @@ static const struct drm_plane_funcs tegra_primary_plane_funcs = {
 };
 
 static int tegra_plane_prepare_fb(struct drm_plane *plane,
-				  struct drm_framebuffer *fb,
 				  const struct drm_plane_state *new_state)
 {
 	return 0;
 }
 
 static void tegra_plane_cleanup_fb(struct drm_plane *plane,
-				   struct drm_framebuffer *fb,
 				   const struct drm_plane_state *old_fb)
 {
 }
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 96e16283afb9..5ffca4e51e40 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -59,10 +59,8 @@ extern int drm_crtc_init(struct drm_device *dev,
  */
 struct drm_plane_helper_funcs {
 	int (*prepare_fb)(struct drm_plane *plane,
-			  struct drm_framebuffer *fb,
 			  const struct drm_plane_state *new_state);
 	void (*cleanup_fb)(struct drm_plane *plane,
-			   struct drm_framebuffer *fb,
 			   const struct drm_plane_state *old_state);
 
 	int (*atomic_check)(struct drm_plane *plane,

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

Re: [PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Rob Clark]

On Thu, Jul 16, 2015 at 10:13 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> This removes the need to separately track fb changes i915.
>
> Changes since v1:
> - Add dri-devel to cc.
> - Fix a check in intel's prepare and cleanup fb to take rotation
>   into account.
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index be9fa8220499..36fda86b3518 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -712,11 +712,13 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p,
>  }
>
>  static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p,
> -                                       struct drm_framebuffer *fb,
>                                         const struct drm_plane_state *new_state)
>  {
>         struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
>
> +       if (!new_state->fb)
> +               return 0;
> +
>         return atmel_hlcdc_layer_update_start(&plane->layer);
>  }
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0898afbc9e23..e52dfc828e60 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1063,17 +1063,14 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>                 const struct drm_plane_helper_funcs *funcs;
>                 struct drm_plane *plane = state->planes[i];
>                 struct drm_plane_state *plane_state = state->plane_states[i];
> -               struct drm_framebuffer *fb;
>
>                 if (!plane)
>                         continue;
>
>                 funcs = plane->helper_private;
>
> -               fb = plane_state->fb;
> -
> -               if (fb && funcs->prepare_fb) {
> -                       ret = funcs->prepare_fb(plane, fb, plane_state);
> +               if (funcs->prepare_fb) {
> +                       ret = funcs->prepare_fb(plane, plane_state);
>                         if (ret)
>                                 goto fail;
>                 }
> @@ -1086,17 +1083,14 @@ fail:
>                 const struct drm_plane_helper_funcs *funcs;
>                 struct drm_plane *plane = state->planes[i];
>                 struct drm_plane_state *plane_state = state->plane_states[i];
> -               struct drm_framebuffer *fb;
>
>                 if (!plane)
>                         continue;
>
>                 funcs = plane->helper_private;
>
> -               fb = state->plane_states[i]->fb;
> -
> -               if (fb && funcs->cleanup_fb)
> -                       funcs->cleanup_fb(plane, fb, plane_state);
> +               if (funcs->cleanup_fb)
> +                       funcs->cleanup_fb(plane, plane_state);
>
>         }
>
> @@ -1252,14 +1246,11 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>
>         for_each_plane_in_state(old_state, plane, plane_state, i) {
>                 const struct drm_plane_helper_funcs *funcs;
> -               struct drm_framebuffer *old_fb;
>
>                 funcs = plane->helper_private;
>
> -               old_fb = plane_state->fb;
> -
> -               if (old_fb && funcs->cleanup_fb)
> -                       funcs->cleanup_fb(plane, old_fb, plane_state);
> +               if (funcs->cleanup_fb)
> +                       funcs->cleanup_fb(plane, plane_state);
>         }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index b07a213f5655..03b7afe455e6 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -425,7 +425,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>
>         if (plane_funcs->prepare_fb && plane_state->fb &&
>             plane_state->fb != old_fb) {
> -               ret = plane_funcs->prepare_fb(plane, plane_state->fb,
> +               ret = plane_funcs->prepare_fb(plane,
>                                               plane_state);
>                 if (ret)
>                         goto out;
> @@ -478,8 +478,8 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>                 ret = 0;
>         }
>
> -       if (plane_funcs->cleanup_fb && old_fb)
> -               plane_funcs->cleanup_fb(plane, old_fb, plane_state);
> +       if (plane_funcs->cleanup_fb)
> +               plane_funcs->cleanup_fb(plane, plane_state);
>  out:
>         if (plane_state) {
>                 if (plane->funcs->atomic_destroy_state)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7dbfeacf0f38..b3990264e137 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4775,17 +4775,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);
> @@ -11668,14 +11657,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;
>
> @@ -13479,21 +13460,26 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>   */
>  int
>  intel_prepare_plane_fb(struct drm_plane *plane,
> -                      struct drm_framebuffer *fb,
>                        const struct drm_plane_state *new_state)
>  {
>         struct drm_device *dev = plane->dev;
> +       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;
> +
> +       if (obj == old_obj && new_state->rotation == plane->state->rotation)
>                 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);
> @@ -13520,21 +13506,29 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   */
>  void
>  intel_cleanup_plane_fb(struct drm_plane *plane,
> -                      struct drm_framebuffer *fb,
>                        const struct drm_plane_state *old_state)
>  {
>         struct drm_device *dev = plane->dev;
> -       struct drm_i915_gem_object *obj = intel_fb_obj(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->fb);
>
> -       if (WARN_ON(!obj))
> +       if (!obj && !old_obj)
>                 return;
>
> -       if (plane->type != DRM_PLANE_TYPE_CURSOR ||
> -           !INTEL_INFO(dev)->cursor_needs_physical) {
> -               mutex_lock(&dev->struct_mutex);
> -               intel_unpin_fb_obj(fb, old_state);
> -               mutex_unlock(&dev->struct_mutex);
> -       }
> +       if (obj == old_obj && old_state->rotation == plane->state->rotation)
> +               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);
> +
> +       /* 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 3b00d00c0bc0..bee8a9669482 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -497,7 +497,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;
> @@ -1039,10 +1038,8 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
>  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>  void intel_check_page_flip(struct drm_device *dev, int pipe);
>  int intel_prepare_plane_fb(struct drm_plane *plane,
> -                          struct drm_framebuffer *fb,
>                            const struct drm_plane_state *new_state);
>  void intel_cleanup_plane_fb(struct drm_plane *plane,
> -                           struct drm_framebuffer *fb,
>                             const struct drm_plane_state *old_state);
>  int intel_plane_atomic_get_property(struct drm_plane *plane,
>                                     const struct drm_plane_state *state,
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> index 0d1dbb737933..60e83c3765c2 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> @@ -98,22 +98,28 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
>  };
>
>  static int mdp4_plane_prepare_fb(struct drm_plane *plane,
> -               struct drm_framebuffer *fb,
>                 const struct drm_plane_state *new_state)
>  {
>         struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
>         struct mdp4_kms *mdp4_kms = get_kms(plane);
> +       struct drm_framebuffer *fb = new_state->fb;
> +
> +       if (!fb)
> +               return 0;
>
>         DBG("%s: prepare: FB[%u]", mdp4_plane->name, fb->base.id);
>         return msm_framebuffer_prepare(fb, mdp4_kms->id);
>  }
>
>  static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
> -               struct drm_framebuffer *fb,
>                 const struct drm_plane_state *old_state)
>  {
>         struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
>         struct mdp4_kms *mdp4_kms = get_kms(plane);
> +       struct drm_framebuffer *fb = old_state->fb;
> +
> +       if (!fb)
> +               return;
>
>         DBG("%s: cleanup: FB[%u]", mdp4_plane->name, fb->base.id);
>         msm_framebuffer_cleanup(fb, mdp4_kms->id);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 57b8f56ae9d0..d0f627b962f3 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -156,11 +156,14 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
>  };
>
>  static int mdp5_plane_prepare_fb(struct drm_plane *plane,
> -               struct drm_framebuffer *fb,
>                 const struct drm_plane_state *new_state)
>  {
>         struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
>         struct mdp5_kms *mdp5_kms = get_kms(plane);
> +       struct drm_framebuffer *fb = new_state->fb;
> +
> +       if (!new_state->fb)
> +               return 0;
>
>         DBG("%s: prepare: FB[%u]", mdp5_plane->name, fb->base.id);
>         return msm_framebuffer_prepare(fb, mdp5_kms->id);
> @@ -172,6 +175,10 @@ static void mdp5_plane_cleanup_fb(struct drm_plane *plane,
>  {
>         struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
>         struct mdp5_kms *mdp5_kms = get_kms(plane);
> +       struct drm_framebuffer *fb = old_state->fb;
> +
> +       if (!fb)
> +               return;
>
>         DBG("%s: cleanup: FB[%u]", mdp5_plane->name, fb->base.id);
>         msm_framebuffer_cleanup(fb, mdp5_kms->id);
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 098904696a5c..09e363bb55f2 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -60,17 +60,19 @@ to_omap_plane_state(struct drm_plane_state *state)
>  }
>
>  static int omap_plane_prepare_fb(struct drm_plane *plane,
> -                                struct drm_framebuffer *fb,
>                                  const struct drm_plane_state *new_state)
>  {
> -       return omap_framebuffer_pin(fb);
> +       if (!new_state->fb)
> +               return 0;
> +
> +       return omap_framebuffer_pin(new_state->fb);
>  }
>
>  static void omap_plane_cleanup_fb(struct drm_plane *plane,
> -                                 struct drm_framebuffer *fb,
>                                   const struct drm_plane_state *old_state)
>  {
> -       omap_framebuffer_unpin(fb);
> +       if (old_state->fb)
> +               omap_framebuffer_unpin(old_state->fb);
>  }
>
>  static void omap_plane_atomic_update(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/sti/sti_drm_plane.c b/drivers/gpu/drm/sti/sti_drm_plane.c
> index 64d4ed43dda3..f3717292e748 100644
> --- a/drivers/gpu/drm/sti/sti_drm_plane.c
> +++ b/drivers/gpu/drm/sti/sti_drm_plane.c
> @@ -147,14 +147,12 @@ static struct drm_plane_funcs sti_drm_plane_funcs = {
>  };
>
>  static int sti_drm_plane_prepare_fb(struct drm_plane *plane,
> -                                 struct drm_framebuffer *fb,
>                                   const struct drm_plane_state *new_state)
>  {
>         return 0;
>  }
>
>  static void sti_drm_plane_cleanup_fb(struct drm_plane *plane,
> -                                  struct drm_framebuffer *fb,
>                                    const struct drm_plane_state *old_fb)
>  {
>  }
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index a287e4fec865..d447701173e6 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -472,14 +472,12 @@ static const struct drm_plane_funcs tegra_primary_plane_funcs = {
>  };
>
>  static int tegra_plane_prepare_fb(struct drm_plane *plane,
> -                                 struct drm_framebuffer *fb,
>                                   const struct drm_plane_state *new_state)
>  {
>         return 0;
>  }
>
>  static void tegra_plane_cleanup_fb(struct drm_plane *plane,
> -                                  struct drm_framebuffer *fb,
>                                    const struct drm_plane_state *old_fb)
>  {
>  }
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 96e16283afb9..5ffca4e51e40 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -59,10 +59,8 @@ extern int drm_crtc_init(struct drm_device *dev,
>   */
>  struct drm_plane_helper_funcs {
>         int (*prepare_fb)(struct drm_plane *plane,
> -                         struct drm_framebuffer *fb,
>                           const struct drm_plane_state *new_state);
>         void (*cleanup_fb)(struct drm_plane *plane,
> -                          struct drm_framebuffer *fb,
>                            const struct drm_plane_state *old_state);
>
>         int (*atomic_check)(struct drm_plane *plane,
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Daniel Vetter]

On Wed, Jul 22, 2015 at 09:23:27AM -0400, Rob Clark wrote:
> On Thu, Jul 16, 2015 at 10:13 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
> > This removes the need to separately track fb changes i915.
> >
> > Changes since v1:
> > - Add dri-devel to cc.
> > - Fix a check in intel's prepare and cleanup fb to take rotation
> >   into account.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Maarten Lankhorst]

Op 27-07-15 om 09:53 schreef Daniel Vetter:
> On Wed, Jul 22, 2015 at 09:23:27AM -0400, Rob Clark wrote:
>> On Thu, Jul 16, 2015 at 10:13 AM, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>> This removes the need to separately track fb changes i915.
>>>
>>> Changes since v1:
>>> - Add dri-devel to cc.
>>> - Fix a check in intel's prepare and cleanup fb to take rotation
>>>   into account.
>>>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Applied to drm-misc, thanks.
> -Daniel
This one needs i915 converted to atomic first, or dpms will fail. :(
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Daniel Vetter]

On Mon, Jul 27, 2015 at 01:07:37PM +0200, Maarten Lankhorst wrote:
> Op 27-07-15 om 09:53 schreef Daniel Vetter:
> > On Wed, Jul 22, 2015 at 09:23:27AM -0400, Rob Clark wrote:
> >> On Thu, Jul 16, 2015 at 10:13 AM, Maarten Lankhorst
> >> <maarten.lankhorst@linux.intel.com> wrote:
> >>> This removes the need to separately track fb changes i915.
> >>>
> >>> Changes since v1:
> >>> - Add dri-devel to cc.
> >>> - Fix a check in intel's prepare and cleanup fb to take rotation
> >>>   into account.
> >>>
> >>> Cc: dri-devel@lists.freedesktop.org
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Reviewed-by: Rob Clark <robdclark@gmail.com>
> > Applied to drm-misc, thanks.
> > -Daniel
> This one needs i915 converted to atomic first, or dpms will fail. :(

Yeah I dropped it again, since it also does some shuffling in i915 code.
That should be done in a split-out patch so that this one here really only
removes the fb parameter.
-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: [Intel-gfx] [PATCH v1.1 2/5] drm/atomic: Make prepare_fb/cleanup_fb only take state, v2.

[Maarten Lankhorst]

Hey,

Op 27-07-15 om 13:14 schreef Daniel Vetter:
> On Mon, Jul 27, 2015 at 01:07:37PM +0200, Maarten Lankhorst wrote:
>> Op 27-07-15 om 09:53 schreef Daniel Vetter:
>>> On Wed, Jul 22, 2015 at 09:23:27AM -0400, Rob Clark wrote:
>>>> On Thu, Jul 16, 2015 at 10:13 AM, Maarten Lankhorst
>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>> This removes the need to separately track fb changes i915.
>>>>>
>>>>> Changes since v1:
>>>>> - Add dri-devel to cc.
>>>>> - Fix a check in intel's prepare and cleanup fb to take rotation
>>>>>   into account.
>>>>>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>> Applied to drm-misc, thanks.
>>> -Daniel
>> This one needs i915 converted to atomic first, or dpms will fail. :(
> Yeah I dropped it again, since it also does some shuffling in i915 code.
> That should be done in a split-out patch so that this one here really only
> removes the fb parameter.
> -Daniel
The code shuffling is a consequence of removing the fb parameter and allowing NULL.

If you want to separate it that's possible, but means returning early in prepare_plane_fb while a followup patch copes with a nil fb.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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 3f82214b737b..d3a857ce1cb6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2383,11 +2383,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
@@ -2403,14 +2402,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;
 }
@@ -13481,7 +13478,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 4/5] drm/i915: Remove wait_for_pending_flips from disable_noatomic.

[Maarten Lankhorst]

Now that intel_display_suspend is atomic it's safe to remove
wait_for_pending_flips from intel_crtc_disable_noatomic. It
will only be used during hw load or resume, in which case there
will be no pending flips anyway.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d3a857ce1cb6..5a462e9a4d14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6187,10 +6187,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	if (to_intel_plane_state(crtc->primary->state)->visible) {
-		intel_crtc_wait_for_pending_flips(crtc);
+	if (to_intel_plane_state(crtc->primary->state)->visible)
 		intel_pre_disable_primary(crtc);
-	}
 
 	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
 	dev_priv->display.crtc_disable(crtc);
-- 
2.1.0

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

Re: [PATCH 4/5] drm/i915: Remove wait_for_pending_flips from disable_noatomic.

[Chris Wilson]

On Thu, Jul 16, 2015 at 02:57:50PM +0200, Maarten Lankhorst wrote:
> Now that intel_display_suspend is atomic it's safe to remove
> wait_for_pending_flips from intel_crtc_disable_noatomic. It
> will only be used during hw load or resume, in which case there
> will be no pending flips anyway.

A WARN_ON(pending_flip) then? (Actually we should start doing
DRM_ERROR_ON I guess that would make a lot of complaints go away, and
also a lot of genuine bug reports) Or do we have warning coverage
elsewhere along the CRTC change path?
-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 4/5] drm/i915: Remove wait_for_pending_flips from disable_noatomic.

[Maarten Lankhorst]

Op 21-07-15 om 13:35 schreef Chris Wilson:
> On Thu, Jul 16, 2015 at 02:57:50PM +0200, Maarten Lankhorst wrote:
>> Now that intel_display_suspend is atomic it's safe to remove
>> wait_for_pending_flips from intel_crtc_disable_noatomic. It
>> will only be used during hw load or resume, in which case there
>> will be no pending flips anyway.
> A WARN_ON(pending_flip) then? (Actually we should start doing
> DRM_ERROR_ON I guess that would make a lot of complaints go away, and
> also a lot of genuine bug reports) Or do we have warning coverage
> elsewhere along the CRTC change path?

intel_sanitize_crtc, called during hw readout, is the only caller of intel_crtc_disable_noatomic.
During hw readout no sw updates should be queued anyway..

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

Re: [PATCH 4/5] drm/i915: Remove wait_for_pending_flips from disable_noatomic.

[Chris Wilson]

On Tue, Jul 21, 2015 at 02:38:14PM +0200, Maarten Lankhorst wrote:
> Op 21-07-15 om 13:35 schreef Chris Wilson:
> > On Thu, Jul 16, 2015 at 02:57:50PM +0200, Maarten Lankhorst wrote:
> >> Now that intel_display_suspend is atomic it's safe to remove
> >> wait_for_pending_flips from intel_crtc_disable_noatomic. It
> >> will only be used during hw load or resume, in which case there
> >> will be no pending flips anyway.
> > A WARN_ON(pending_flip) then? (Actually we should start doing
> > DRM_ERROR_ON I guess that would make a lot of complaints go away, and
> > also a lot of genuine bug reports) Or do we have warning coverage
> > elsewhere along the CRTC change path?
> 
> intel_sanitize_crtc, called during hw readout, is the only caller of intel_crtc_disable_noatomic.
> During hw readout no sw updates should be queued anyway..

Is there any documentation to say that this function can't ever be
called outside of sanitize_crtc? Perhaps rename the function to reflect
its usage?
-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 4/5] drm/i915: Remove wait_for_pending_flips from disable_noatomic.

[Maarten Lankhorst]

Op 21-07-15 om 14:40 schreef Chris Wilson:
> On Tue, Jul 21, 2015 at 02:38:14PM +0200, Maarten Lankhorst wrote:
>> Op 21-07-15 om 13:35 schreef Chris Wilson:
>>> On Thu, Jul 16, 2015 at 02:57:50PM +0200, Maarten Lankhorst wrote:
>>>> Now that intel_display_suspend is atomic it's safe to remove
>>>> wait_for_pending_flips from intel_crtc_disable_noatomic. It
>>>> will only be used during hw load or resume, in which case there
>>>> will be no pending flips anyway.
>>> A WARN_ON(pending_flip) then? (Actually we should start doing
>>> DRM_ERROR_ON I guess that would make a lot of complaints go away, and
>>> also a lot of genuine bug reports) Or do we have warning coverage
>>> elsewhere along the CRTC change path?
>> intel_sanitize_crtc, called during hw readout, is the only caller of intel_crtc_disable_noatomic.
>> During hw readout no sw updates should be queued anyway..
> Is there any documentation to say that this function can't ever be
> called outside of sanitize_crtc? Perhaps rename the function to reflect
> its usage?

There's no documentation, but all updates outside this function are done with atomic updates
and this function is the only one that doesn't update the atomic state.

If you use it, you better have a good reason to.

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

[PATCH 5/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 | 72 +++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 -
 3 files changed, 33 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e2531cf59266..09a0ad611002 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3250,32 +3250,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;
@@ -3890,15 +3864,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);
@@ -3909,11 +3891,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 */
@@ -4773,9 +4751,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);
 
@@ -11701,7 +11676,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;
 
@@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
 	int ret = 0;
 
+	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
+		ret = intel_crtc_wait_for_pending_flips(plane->state->crtc);
+		if (ret)
+			return ret;
+	}
+
 	if (obj == old_obj)
 		return 0;
 
@@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
 	}
 
+	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
+		ret = i915_gem_object_wait_rendering(old_obj, true);
+
 	if (ret == 0)
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e6b04941f9ca..0920a6fc459a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -489,7 +489,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;
@@ -1126,7 +1125,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

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

[Maarten Lankhorst]

Op 16-07-15 om 14:57 schreef 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 | 72 +++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>  3 files changed, 33 insertions(+), 43 deletions(-)
>
This breaks DPMS off, because it might wait for flips before continuing.
It looks like all drivers use drmModeConnectorSetProperty correctly,
so it's safe to fix dpms to return an error.

Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series?

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

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

[Daniel Vetter]

On Mon, Jul 20, 2015 at 04:52:11PM +0200, Maarten Lankhorst wrote:
> Op 16-07-15 om 14:57 schreef 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 | 72 +++++++++++++++++-------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 -
> >  3 files changed, 33 insertions(+), 43 deletions(-)
> >
> This breaks DPMS off, because it might wait for flips before continuing.
> It looks like all drivers use drmModeConnectorSetProperty correctly,
> so it's safe to fix dpms to return an error.
> 
> Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series?

Yeah makes sense.
-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 5/5] drm/i915: Make wait_for_flips interruptible.

[Maarten Lankhorst]

Op 21-07-15 om 08:51 schreef Daniel Vetter:
> On Mon, Jul 20, 2015 at 04:52:11PM +0200, Maarten Lankhorst wrote:
>> Op 16-07-15 om 14:57 schreef 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 | 72 +++++++++++++++++-------------------
>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>>>  3 files changed, 33 insertions(+), 43 deletions(-)
>>>
>> This breaks DPMS off, because it might wait for flips before continuing.
>> It looks like all drivers use drmModeConnectorSetProperty correctly,
>> so it's safe to fix dpms to return an error.
>>
>> Can I get the changes to dpms handling in intel applied first, and the first 4 patches from this series?
>
Ok patch sent to dri-devel, after dpms returns a value this patch should be safe to apply.

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

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

[Chris Wilson]

On Thu, Jul 16, 2015 at 02:57:51PM +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 | 72 +++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>  3 files changed, 33 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index e2531cf59266..09a0ad611002 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3250,32 +3250,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;
> @@ -3890,15 +3864,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);
> @@ -3909,11 +3891,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 */
> @@ -4773,9 +4751,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);
>  
> @@ -11701,7 +11676,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;
>  
> @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>  	int ret = 0;
>  
> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {

Would feel safer is we just asked if the CRTC had pending flips
irrespective of old_obj. Do you plan on moving the pending flips from
CRTC to plane? That would seem to be implied here.

> +		ret = intel_crtc_wait_for_pending_flips(plane->state->crtc);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (obj == old_obj)
>  		return 0;
>  
> @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
>  	}
>  
> +	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
> +		ret = i915_gem_object_wait_rendering(old_obj, true);

Technically I can create a batch with a WAIT_ON_EVENT for a secondary
plane as well. This path need only be done in a modeset, not for a
simple plane flip. (But we actually need to serialise with rendering to
old_obj before regarding the plane flip as complete.) Is there a
plane-prepare-modeset hook?
-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: Make wait_for_flips interruptible.

[Maarten Lankhorst]

Op 21-07-15 om 13:26 schreef Chris Wilson:
> On Thu, Jul 16, 2015 at 02:57:51PM +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 | 72 +++++++++++++++++-------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>>  3 files changed, 33 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index e2531cf59266..09a0ad611002 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3250,32 +3250,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;
>> @@ -3890,15 +3864,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);
>> @@ -3909,11 +3891,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 */
>> @@ -4773,9 +4751,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);
>>  
>> @@ -11701,7 +11676,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;
>>  
>> @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>>  	int ret = 0;
>>  
>> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
> Would feel safer is we just asked if the CRTC had pending flips
> irrespective of old_obj. Do you plan on moving the pending flips from
> CRTC to plane? That would seem to be implied here.
This preserves old behavior since page flips can only happen on primary planes.
Do you want cursor updates to wait on pending primary flips?

>> +		ret = intel_crtc_wait_for_pending_flips(plane->state->crtc);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	if (obj == old_obj)
>>  		return 0;
>>  
>> @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
>>  	}
>>  
>> +	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
>> +		ret = i915_gem_object_wait_rendering(old_obj, true);
> Technically I can create a batch with a WAIT_ON_EVENT for a secondary
> plane as well. This path need only be done in a modeset, not for a
> simple plane flip. (But we actually need to serialise with rendering to
> old_obj before regarding the plane flip as complete.) Is there a
> plane-prepare-modeset hook?
The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here?

~Maarten

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

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

[Chris Wilson]

On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote:
> >> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
> > Would feel safer is we just asked if the CRTC had pending flips
> > irrespective of old_obj. Do you plan on moving the pending flips from
> > CRTC to plane? That would seem to be implied here.
> This preserves old behavior since page flips can only happen on primary planes.
> Do you want cursor updates to wait on pending primary flips?

Cursor updates need to be serialized with primary updates, yes (or else
there are some fun scenarios in which you can hang the display engine).

What I was actually thinking was using plane->state for tracking the
flips. Having just reread what I wrote that didn't come across. I would
rather see fewer primary_plane special cases and more no-ops on
secondary planes. I think you will end up using such infrastructure
on the secondary planes eventually.

> >> +	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
> >> +		ret = i915_gem_object_wait_rendering(old_obj, true);
> > Technically I can create a batch with a WAIT_ON_EVENT for a secondary
> > plane as well. This path need only be done in a modeset, not for a
> > simple plane flip. (But we actually need to serialise with rendering to
> > old_obj before regarding the plane flip as complete.) Is there a
> > plane-prepare-modeset hook?
> The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here?

For the purpose of limiting the wait to when the pipe dimensions may
change, yes.
-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: Make wait_for_flips interruptible.

[Maarten Lankhorst]

Op 21-07-15 om 15:31 schreef Chris Wilson:
> On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote:
>>>> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
>>> Would feel safer is we just asked if the CRTC had pending flips
>>> irrespective of old_obj. Do you plan on moving the pending flips from
>>> CRTC to plane? That would seem to be implied here.
>> This preserves old behavior since page flips can only happen on primary planes.
>> Do you want cursor updates to wait on pending primary flips?
> Cursor updates need to be serialized with primary updates, yes (or else
> there are some fun scenarios in which you can hang the display engine).
Ok but right now we don't sync this.. won't blocking on flips for moving the mouse in Xorg be bad for performance?
> What I was actually thinking was using plane->state for tracking the
> flips. Having just reread what I wrote that didn't come across. I would
> rather see fewer primary_plane special cases and more no-ops on
> secondary planes. I think you will end up using such infrastructure
> on the secondary planes eventually.
I think the current infrastructure will die eventually and perhaps be done asynchronously. I just don't know yet what will replace it.

>>>> +	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
>>>> +		ret = i915_gem_object_wait_rendering(old_obj, true);
>>> Technically I can create a batch with a WAIT_ON_EVENT for a secondary
>>> plane as well. This path need only be done in a modeset, not for a
>>> simple plane flip. (But we actually need to serialise with rendering to
>>> old_obj before regarding the plane flip as complete.) Is there a
>>> plane-prepare-modeset hook?
>> The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here?
> For the purpose of limiting the wait to when the pipe dimensions may
> change, yes.
Is that only needed during full modesets, or when changing PIPESRC 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: Make wait_for_flips interruptible.

[Chris Wilson]

On Tue, Jul 21, 2015 at 03:59:25PM +0200, Maarten Lankhorst wrote:
> Op 21-07-15 om 15:31 schreef Chris Wilson:
> > On Tue, Jul 21, 2015 at 02:33:33PM +0200, Maarten Lankhorst wrote:
> >>>> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
> >>> Would feel safer is we just asked if the CRTC had pending flips
> >>> irrespective of old_obj. Do you plan on moving the pending flips from
> >>> CRTC to plane? That would seem to be implied here.
> >> This preserves old behavior since page flips can only happen on primary planes.
> >> Do you want cursor updates to wait on pending primary flips?
> > Cursor updates need to be serialized with primary updates, yes (or else
> > there are some fun scenarios in which you can hang the display engine).
> Ok but right now we don't sync this.. won't blocking on flips for moving the mouse in Xorg be bad for performance?

Off the top of my head, we only need to worry about modesets, which 
presumedly are serialized between the planes.

> > What I was actually thinking was using plane->state for tracking the
> > flips. Having just reread what I wrote that didn't come across. I would
> > rather see fewer primary_plane special cases and more no-ops on
> > secondary planes. I think you will end up using such infrastructure
> > on the secondary planes eventually.
> I think the current infrastructure will die eventually and perhaps be done asynchronously. I just don't know yet what will replace it.
> 
> >>>> +	/* 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 (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
> >>>> +		ret = i915_gem_object_wait_rendering(old_obj, true);
> >>> Technically I can create a batch with a WAIT_ON_EVENT for a secondary
> >>> plane as well. This path need only be done in a modeset, not for a
> >>> simple plane flip. (But we actually need to serialise with rendering to
> >>> old_obj before regarding the plane flip as complete.) Is there a
> >>> plane-prepare-modeset hook?
> >> The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here?
> > For the purpose of limiting the wait to when the pipe dimensions may
> > change, yes.
> Is that only needed during full modesets, or when changing PIPESRC too?

The triggers are the display modeline values (whatever HTOTAL/VTOTAL
registers collectively are called), so just full modesets.
-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