Skip to contentSkip to conversation

drm/i915: create a prepare step for primary planes updates

PATCH Applied Gustavo Padovan 7 participants 24 messages Oct 24, 2014 – Nov 12, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v4 1/5 drm/i915: create a prepare step for primary planes updates

From: Gustavo Padovan <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)
drivers/gpu/drm/i915/intel_display.c +26 -9
index 7d891e5..8530401 100644
@@ -11660,20 +11660,16 @@ intel_check_primary_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_primary_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
+intel_prepare_primary_plane(struct drm_plane *plane,
+			    struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *old_fb = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_rect *src = &state->src;
 	int ret;
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11683,7 +11679,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		return -EBUSY;
 	}
 
-	if (plane->fb != fb) {
+	if (old_obj != obj) {
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 		if (ret == 0)
@@ -11696,6 +11692,25 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		}
 	}
 
+	return 0;
+}
+
+static void
+intel_commit_primary_plane(struct drm_plane *plane,
+			   struct intel_plane_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *old_fb = plane->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_rect *src = &state->src;
+
 	crtc->primary->fb = fb;
 	crtc->x = src->x1;
 	crtc->y = src->y1;
@@ -11772,8 +11787,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -11814,6 +11827,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	ret = intel_prepare_primary_plane(plane, &state);
+	if (ret)
+		return ret;
+
 	intel_commit_primary_plane(plane, &state);
 
 	return 0;
Signature · 6 lines
1.9.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

PATCH v4 2/5 drm/i915: create a prepare phase for sprite plane updates

From: Gustavo Padovan <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +40 -23
index 2c060ad..3631b0e 100644
@@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_sprite_plane(struct drm_plane *plane,
-			  struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+			   struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *dst = &state->dst;
-	const struct drm_rect *clip = &state->clip;
-	bool primary_enabled;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	int ret;
 
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
 	if (old_obj != obj) {
 		mutex_lock(&dev->struct_mutex);
 
@@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			return ret;
 	}
 
+	return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+			  struct intel_plane_state *state)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_crtc *crtc = state->crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *fb = state->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	int crtc_x, crtc_y;
+	unsigned int crtc_w, crtc_h;
+	uint32_t src_x, src_y, src_w, src_h;
+	struct drm_rect *dst = &state->dst;
+	const struct drm_rect *clip = &state->clip;
+	bool primary_enabled;
+
+	/*
+	 * If the sprite is completely covering the primary plane,
+	 * we can disable the primary and save power.
+	 */
+	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
 	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_obj);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	return 0;
 }
 
 static int
@@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	return intel_commit_sprite_plane(plane, &state);
+	ret = intel_prepare_sprite_plane(plane, &state);
+	if (ret)
+		return ret;
+
+	intel_commit_sprite_plane(plane, &state);
+	return 0;
 }
 
 static int
Signature · 6 lines
1.9.3

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

Re: [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates

2014-10-24 11:51 GMT-02:00 Gustavo Padovan <[email protected]>:

Quoted 4 lines

From: Gustavo Padovan <[email protected]>

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

According to my bisection results, this is the first bad commit of
https://bugs.freedesktop.org/show_bug.cgi?id=85634.

Quoted 116 lines
---
 drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +40 -24
index 2c060ad..3631b0e 100644
@@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_sprite_plane(struct drm_plane *plane,
-                         struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+                          struct intel_plane_state *state)
 {
        struct drm_device *dev = plane->dev;
        struct drm_crtc *crtc = state->crtc;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-       struct intel_plane *intel_plane = to_intel_plane(plane);
        enum pipe pipe = intel_crtc->pipe;
        struct drm_framebuffer *fb = state->fb;
-       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-       struct drm_i915_gem_object *obj = intel_fb->obj;
-       struct drm_i915_gem_object *old_obj = intel_plane->obj;
-       int crtc_x, crtc_y;
-       unsigned int crtc_w, crtc_h;
-       uint32_t src_x, src_y, src_w, src_h;
-       struct drm_rect *dst = &state->dst;
-       const struct drm_rect *clip = &state->clip;
-       bool primary_enabled;
+       struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+       struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
        int ret;
 
-       /*
-        * If the sprite is completely covering the primary plane,
-        * we can disable the primary and save power.
-        */
-       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
        if (old_obj != obj) {
                mutex_lock(&dev->struct_mutex);
 
@@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                        return ret;
        }
 
+       return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+                         struct intel_plane_state *state)
+{
+       struct drm_device *dev = plane->dev;
+       struct drm_crtc *crtc = state->crtc;
+       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_plane *intel_plane = to_intel_plane(plane);
+       enum pipe pipe = intel_crtc->pipe;
+       struct drm_framebuffer *fb = state->fb;
+       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+       struct drm_i915_gem_object *obj = intel_fb->obj;
+       struct drm_i915_gem_object *old_obj = intel_plane->obj;
+       int crtc_x, crtc_y;
+       unsigned int crtc_w, crtc_h;
+       uint32_t src_x, src_y, src_w, src_h;
+       struct drm_rect *dst = &state->dst;
+       const struct drm_rect *clip = &state->clip;
+       bool primary_enabled;
+
+       /*
+        * If the sprite is completely covering the primary plane,
+        * we can disable the primary and save power.
+        */
+       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
        intel_plane->crtc_x = state->orig_dst.x1;
        intel_plane->crtc_y = state->orig_dst.y1;
        intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                intel_unpin_fb_obj(old_obj);
                mutex_unlock(&dev->struct_mutex);
        }
-
-       return 0;
 }
 
 static int
@@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
        if (ret)
                return ret;
 
-       return intel_commit_sprite_plane(plane, &state);
+       ret = intel_prepare_sprite_plane(plane, &state);
+       if (ret)
+               return ret;
+
+       intel_commit_sprite_plane(plane, &state);
+       return 0;
 }
 
 static int
--

1.9.3

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

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#3 lore raw

Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates

Quoted 8 lines · Paulo Zanoni

2014-10-24 11:51 GMT-02:00 Gustavo Padovan <[email protected]>:

Quoted 4 lines

From: Gustavo Padovan <[email protected]>

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

According to my bisection results, this is the first bad commit of
https://bugs.freedesktop.org/show_bug.cgi?id=85634.

I saw the same issue, when checking #82939. AFAICS when disabling the crtc intel_plane->obj will be set to NULL and the object will be unpinned, but plane->fb remains set. Next if drm_set_plane with the same fb is called intel_prepare_sprite_plane() won't pin the object since it sees that same fb is still set, but sets intel_plane->obj. Next if drm_set_plane called with a NULL fb, it will try unpin the object, and the unbalanced refcount throws a WARN. The following got rid of the problem for me:

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_sprite.c
index 8b80d68..36762b5 100644

drivers/gpu/drm/i915/intel_sprite.c +0 -0
@@ -1198,9 +1198,10 @@ intel_prepare_sprite_plane(struct drm_plane

*plane,
struct drm_crtc *crtc = state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc->pipe;
+ struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_framebuffer *fb = state->fb; 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_plane->obj;
int ret;

if (old_obj != obj) {

Quoted 110 lines
Quoted 110 lines
---
 drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +40 -23
index 2c060ad..3631b0e 100644
@@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_sprite_plane(struct drm_plane *plane,
-                         struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+                          struct intel_plane_state *state)
 {
        struct drm_device *dev = plane->dev;
        struct drm_crtc *crtc = state->crtc;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-       struct intel_plane *intel_plane = to_intel_plane(plane);
        enum pipe pipe = intel_crtc->pipe;
        struct drm_framebuffer *fb = state->fb;
-       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-       struct drm_i915_gem_object *obj = intel_fb->obj;
-       struct drm_i915_gem_object *old_obj = intel_plane->obj;
-       int crtc_x, crtc_y;
-       unsigned int crtc_w, crtc_h;
-       uint32_t src_x, src_y, src_w, src_h;
-       struct drm_rect *dst = &state->dst;
-       const struct drm_rect *clip = &state->clip;
-       bool primary_enabled;
+       struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+       struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
        int ret;
 
-       /*
-        * If the sprite is completely covering the primary plane,
-        * we can disable the primary and save power.
-        */
-       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
        if (old_obj != obj) {
                mutex_lock(&dev->struct_mutex);
 
@@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                        return ret;
        }
 
+       return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+                         struct intel_plane_state *state)
+{
+       struct drm_device *dev = plane->dev;
+       struct drm_crtc *crtc = state->crtc;
+       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_plane *intel_plane = to_intel_plane(plane);
+       enum pipe pipe = intel_crtc->pipe;
+       struct drm_framebuffer *fb = state->fb;
+       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+       struct drm_i915_gem_object *obj = intel_fb->obj;
+       struct drm_i915_gem_object *old_obj = intel_plane->obj;
+       int crtc_x, crtc_y;
+       unsigned int crtc_w, crtc_h;
+       uint32_t src_x, src_y, src_w, src_h;
+       struct drm_rect *dst = &state->dst;
+       const struct drm_rect *clip = &state->clip;
+       bool primary_enabled;
+
+       /*
+        * If the sprite is completely covering the primary plane,
+        * we can disable the primary and save power.
+        */
+       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
        intel_plane->crtc_x = state->orig_dst.x1;
        intel_plane->crtc_y = state->orig_dst.y1;
        intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                intel_unpin_fb_obj(old_obj);
                mutex_unlock(&dev->struct_mutex);
        }
-
-       return 0;
 }
 
 static int
@@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
        if (ret)
                return ret;
 
-       return intel_commit_sprite_plane(plane, &state);
+       ret = intel_prepare_sprite_plane(plane, &state);
+       if (ret)
+               return ret;
+
+       intel_commit_sprite_plane(plane, &state);
+       return 0;
 }
 
Quoted 11 lines
Quoted 8 lines
drivers/gpu/drm/i915/intel_sprite.c +0 -1
 static int
--

1.9.3

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

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#4 lore raw

Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates

In reply to Paulo Zanoni
Quoted 28 lines · Paulo Zanoni

2014-10-24 11:51 GMT-02:00 Gustavo Padovan <[email protected]>:

Quoted 4 lines

From: Gustavo Padovan <[email protected]>

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

According to my bisection results, this is the first bad commit of
https://bugs.freedesktop.org/show_bug.cgi?id=85634.

Quoted 18 lines
---
 drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +2 -2
index 2c060ad..3631b0e 100644
@@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_commit_sprite_plane(struct drm_plane *plane,
-                         struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+                          struct intel_plane_state *state)

<snip>

Quoted 2 lines

drivers/gpu/drm/i915/intel_sprite.c intel_check_sprite_plane(struct drm_plane *plane, · line 1204

Quoted 2 lines
drivers/gpu/drm/i915/intel_sprite.c +1 -1
-       struct drm_i915_gem_object *old_obj = intel_plane->obj;
+       struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);

I think this change is the culprit here. The problem is that during a modeset we call intel_plane_disable()->intel_disable_plane() (yeah, yeah the names suck big time :) which will unpin the current buffer and clear intel_plane->obj, but plane->fb will still be set, so the next time .update_plane() is called it will think the buffer was already pinned, and not attempt to pin it again.

So just using intel_plane->obj to figure out the old obj should fix it. But my plan sort of was that even if userspace "enables" a plane with the crtc off, we'd still pin the buffer. Then we can be more sure that when the crtc does get enabled the plane can actually be enabled as well (ie. the pinning can't fail at that point anymore). So if we follow that design I think we need to split intel_disable_plane() into two parts, one which just does the plane disable, and the second part just does unpinning. And then we need to just call the non-unpin variant from intel_plane_disable() so that we don't unpin the buffer during crtc disable. And then we should also kill intel_plane->obj.

Quoted 89 lines
Quoted 80 lines
        int ret;
drivers/gpu/drm/i915/intel_sprite.c +36 -12
-       /*
-        * If the sprite is completely covering the primary plane,
-        * we can disable the primary and save power.
-        */
-       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
        if (old_obj != obj) {
                mutex_lock(&dev->struct_mutex);
@@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                        return ret;
        }
 
+       return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+                         struct intel_plane_state *state)
+{
+       struct drm_device *dev = plane->dev;
+       struct drm_crtc *crtc = state->crtc;
+       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_plane *intel_plane = to_intel_plane(plane);
+       enum pipe pipe = intel_crtc->pipe;
+       struct drm_framebuffer *fb = state->fb;
+       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+       struct drm_i915_gem_object *obj = intel_fb->obj;
+       struct drm_i915_gem_object *old_obj = intel_plane->obj;
+       int crtc_x, crtc_y;
+       unsigned int crtc_w, crtc_h;
+       uint32_t src_x, src_y, src_w, src_h;
+       struct drm_rect *dst = &state->dst;
+       const struct drm_rect *clip = &state->clip;
+       bool primary_enabled;
+
+       /*
+        * If the sprite is completely covering the primary plane,
+        * we can disable the primary and save power.
+        */
+       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
        intel_plane->crtc_x = state->orig_dst.x1;
        intel_plane->crtc_y = state->orig_dst.y1;
        intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
                intel_unpin_fb_obj(old_obj);
                mutex_unlock(&dev->struct_mutex);
        }
-
-       return 0;
 }
 
 static int
@@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
        if (ret)
                return ret;
 
-       return intel_commit_sprite_plane(plane, &state);
+       ret = intel_prepare_sprite_plane(plane, &state);
+       if (ret)
+               return ret;
+
+       intel_commit_sprite_plane(plane, &state);
+       return 0;
 }
 
 static int
--

1.9.3

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

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#5 lore raw

PATCH 1/1 drm/i915: use the correct obj when preparing the sprite plane

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. This patch should revert the code back to the previous behavior, fixing the regression.

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates

Credits to Imre Deak for pointing out the exact lines that were wrong.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_sprite.c +2 -1
index 8b80d68..1c874309 100644
@@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	struct 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_plane->obj;
 	int ret;
 
 	if (old_obj != obj) {
Signature · 6 lines
2.1.1

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

Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane

Quoted 33 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. This patch should revert the code back to the previous behavior, fixing the regression.

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates

Credits to Imre Deak for pointing out the exact lines that were wrong.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_sprite.c +0 -0
index 8b80d68..1c874309 100644

drivers/gpu/drm/i915/intel_sprite.c intel_prepare_sprite_plane(struct drm_plane *plane, · line 1204

Quoted 9 lines
drivers/gpu/drm/i915/intel_sprite.c +2 -1
@@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	struct 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_plane->obj;

You need to change intel_commit_sprite_plane() too. Othwerwise something like this could happen:

1. .update_plane(fb=A)
// note plane->fb=NULL here
-> prepare() -> pin(A) -> obj=A
-> commit() -> nop
...
plane->fb=A

2. .crtc_disable()
-> disable() -> unpin(A) -> obj=NULL

3. .update_plane(fb=B)
// note plane->fb=A here still
-> prepare() -> pin(B)
-> commit() -> unpin(A) -> obj=B
...
plane->fb=B

So we still get the double unpin of A.

I take it our basic plane tests didn't catch this?

Quoted 5 lines
drivers/gpu/drm/i915/intel_sprite.c +0 -0
 	int ret;
 
 	if (old_obj != obj) {
Signature · 1 line
2.1.1
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#7 lore raw

Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-10-30 17:10 GMT-02:00 Ville Syrjälä <[email protected]>:

Quoted 64 lines · Paulo Zanoni
Quoted 33 lines

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. This patch should revert the code back to the previous behavior, fixing the regression.

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates

Credits to Imre Deak for pointing out the exact lines that were wrong.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_sprite.c +0 -0
index 8b80d68..1c874309 100644

drivers/gpu/drm/i915/intel_sprite.c intel_prepare_sprite_plane(struct drm_plane *plane, · line 1204

Quoted 9 lines
drivers/gpu/drm/i915/intel_sprite.c +2 -1
@@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
      struct drm_device *dev = plane->dev;
      struct drm_crtc *crtc = state->crtc;
      struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+     struct intel_plane *intel_plane = to_intel_plane(plane);
      enum pipe pipe = intel_crtc->pipe;
      struct drm_framebuffer *fb = state->fb;
      struct 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_plane->obj;

You need to change intel_commit_sprite_plane() too. Othwerwise something like this could happen:

1. .update_plane(fb=A)
// note plane->fb=NULL here
-> prepare() -> pin(A) -> obj=A
-> commit() -> nop
...
plane->fb=A

2. .crtc_disable()
-> disable() -> unpin(A) -> obj=NULL

3. .update_plane(fb=B)
// note plane->fb=A here still
-> prepare() -> pin(B)
-> commit() -> unpin(A) -> obj=B
...
plane->fb=B

Quoted 3 lines

So we still get the double unpin of A.

I take it our basic plane tests didn't catch this?

I didn't run any, nor associated any of the recent bug reports with that. Feel free to write tests if you can think of an easy way to exploit the bug. I also won't mind if someone else writes/sends v2 of this patch :)

Quoted 10 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_sprite.c +0 -1
      int ret;
 
      if (old_obj != obj) {
--

2.1.1

--
Ville Syrjälä
Intel OTC

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#8 lore raw

PATCH 1/1 drm/i915: use the correct obj when preparing the sprite plane

In reply to Ville Syrjälä

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. Later, commit "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced the same problem to function intel_commit_sprite_plane().

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates
and:
commit 77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:33 2014 +0100
drm/i915: use intel_fb_obj() macros to assign gem objects

Credits to Imre Deak for pointing out the exact lines that were wrong.

v2: Also fix intel_commit_sprite_plane() (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +3 -2
index 64076555..7d9c340 100644
@@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	struct 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_plane->obj;
 	int ret;
 
 	if (old_obj != obj) {
@@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	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_plane->obj;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
Signature · 6 lines
2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Reviewed-by on v1 1/1
#9 lore raw

Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane

Quoted 29 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. Later, commit "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced the same problem to function intel_commit_sprite_plane().

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates
and:
commit 77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:33 2014 +0100
drm/i915: use intel_fb_obj() macros to assign gem objects

Credits to Imre Deak for pointing out the exact lines that were wrong.

v2: Also fix intel_commit_sprite_plane() (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Quoted 3 lines
Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>

Yep, I believe that should do it.

As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme.

Quoted 32 lines
---
 drivers/gpu/drm/i915/intel_sprite.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +3 -2
index 64076555..7d9c340 100644
@@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	struct 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_plane->obj;
 	int ret;
 
 	if (old_obj != obj) {
@@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
 	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_plane->obj;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
Signature · 1 line
2.1.1
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#10 lore raw

How to handle planes/fbs when disabling the crtc? (was Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane)

Quoted 7 lines · Ville Syrjälä

As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme.

Hm, I've thought that thus far we've kept the buffer pinned when the crtc is enabled (irrespective or crtc state). And when the crtc gets disabled we dropped the buffers. Then planes happened and everything got messy.

Actually I'm not really sure what the right semantics are here - in the atomic helpers I don't disable planes/framebuffers. Which is consistent with the legacy plane interface, but not consistent with the legacy setCrtc ioctl.

Anyone has a good idea how to handle all this properly? -Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#11 lore raw

Re: How to handle planes/fbs when disabling the crtc? (was Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane)

Quoted 13 lines · Daniel Vetter
Quoted 7 lines · Ville Syrjälä

As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme.

Hm, I've thought that thus far we've kept the buffer pinned when the crtc is enabled (irrespective or crtc state). And when the crtc gets disabled we dropped the buffers. Then planes happened and everything got messy.

Quoted 6 lines

Actually I'm not really sure what the right semantics are here - in the atomic helpers I don't disable planes/framebuffers. Which is consistent with the legacy plane interface, but not consistent with the legacy setCrtc ioctl.

Anyone has a good idea how to handle all this properly?

Well I think we should avoid the "change in property X changes property Y" problem. That means leaving the plane->fb alone when the crtc is disabled.

But as as far as the pinning goes, my original idea was that we keep things pinned as long as plane->fb is set, whether the plane is invisible or crtc disabled. The idea was you could set up all the planes in advance, and then enable the crtc and it would at least not fail due to failure to pin the buffers.

But that is rather wasteful and might prevent defragmenting the address space. So I suppose we should just change things so that at least we don't keep the buffers pinned when the crtc is disabled. And perhaps we should just go all the way and not pin when the plane is invisible, for any reason.

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#12 lore raw

Re: How to handle planes/fbs when disabling the crtc? (was Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane)

Quoted 31 lines · Ville Syrjälä
Quoted 13 lines · Daniel Vetter
Quoted 7 lines · Ville Syrjälä

As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme.

Hm, I've thought that thus far we've kept the buffer pinned when the crtc is enabled (irrespective or crtc state). And when the crtc gets disabled we dropped the buffers. Then planes happened and everything got messy.

Quoted 6 lines

Actually I'm not really sure what the right semantics are here - in the atomic helpers I don't disable planes/framebuffers. Which is consistent with the legacy plane interface, but not consistent with the legacy setCrtc ioctl.

Anyone has a good idea how to handle all this properly?

Well I think we should avoid the "change in property X changes property Y" problem. That means leaving the plane->fb alone when the crtc is disabled.

But as as far as the pinning goes, my original idea was that we keep things pinned as long as plane->fb is set, whether the plane is invisible or crtc disabled. The idea was you could set up all the planes in advance, and then enable the crtc and it would at least not fail due to failure to pin the buffers.

Quoted 5 lines

But that is rather wasteful and might prevent defragmenting the address space. So I suppose we should just change things so that at least we don't keep the buffers pinned when the crtc is disabled. And perhaps we should just go all the way and not pin when the plane is invisible, for any reason.

The problem is a bit that the legacy setCrtc does free the buffer, and userspace might rely on that. So if we decide to keep the plane fb around when the crtc is disabled we need to at least update the set_config atomic helper function to release the fb of the primary plane and unlink/disable the primary plane.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#13 lore raw

Re: How to handle planes/fbs when disabling the crtc? (was Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane)

Quoted 38 lines · Daniel Vetter
Quoted 31 lines · Ville Syrjälä
Quoted 13 lines · Daniel Vetter
Quoted 7 lines · Ville Syrjälä

As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme.

Hm, I've thought that thus far we've kept the buffer pinned when the crtc is enabled (irrespective or crtc state). And when the crtc gets disabled we dropped the buffers. Then planes happened and everything got messy.

Quoted 6 lines

Actually I'm not really sure what the right semantics are here - in the atomic helpers I don't disable planes/framebuffers. Which is consistent with the legacy plane interface, but not consistent with the legacy setCrtc ioctl.

Anyone has a good idea how to handle all this properly?

Well I think we should avoid the "change in property X changes property Y" problem. That means leaving the plane->fb alone when the crtc is disabled.

But as as far as the pinning goes, my original idea was that we keep things pinned as long as plane->fb is set, whether the plane is invisible or crtc disabled. The idea was you could set up all the planes in advance, and then enable the crtc and it would at least not fail due to failure to pin the buffers.

Quoted 5 lines

But that is rather wasteful and might prevent defragmenting the address space. So I suppose we should just change things so that at least we don't keep the buffers pinned when the crtc is disabled. And perhaps we should just go all the way and not pin when the plane is invisible, for any reason.

Quoted 5 lines

The problem is a bit that the legacy setCrtc does free the buffer, and userspace might rely on that. So if we decide to keep the plane fb around when the crtc is disabled we need to at least update the set_config atomic helper function to release the fb of the primary plane and unlink/disable the primary plane.

Sounds reasonable. At least I'd prefer if we can keep such uglies neatly tucked away in their own legacy corner, and not let them spread into the general population.

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#14 lore raw

Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane

In reply to Ville Syrjälä
Quoted 34 lines · Ville Syrjälä
Quoted 29 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. Later, commit "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced the same problem to function intel_commit_sprite_plane().

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates
and:
commit 77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:33 2014 +0100
drm/i915: use intel_fb_obj() macros to assign gem objects

Credits to Imre Deak for pointing out the exact lines that were wrong.

v2: Also fix intel_commit_sprite_plane() (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak <[email protected]>

Quoted 3 lines
Cc Gustavo Padovan <[email protected]>
Cc Ville Syrjälä <[email protected]>
Quoted 3 lines

Yep, I believe that should do it.

Queued for -next, thanks for the patch.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-by on v1 1/1
#15 lore raw

Re: [PATCH] drm/i915: use the correct obj when preparing

In reply to Paulo Zanoni

-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=277/348->277/348
PNV: pass/total=323/328->325/328
ILK: pass/total=328/330->330/330
IVB: pass/total=545/546->544/546
SNB: pass/total=558/563->562/563
HSW: pass/total=587/591->590/591
BDW: pass/total=435/435->434/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
PNV: Intel_gpu_tools, igt_gem_mmap_offset_exhaustion, DMESG_WARN(2, M23M24)PASS(14, M24M23M7) -> PASS(4, M7)
PNV: Intel_gpu_tools, igt_gem_unref_active_buffers, DMESG_WARN(2, M23)PASS(14, M24M23M7) -> PASS(4, M7)
ILK: Intel_gpu_tools, igt_kms_render_gpu-blit, DMESG_WARN(1, M26)PASS(15, M6M26M37) -> PASS(4, M6)
ILK: Intel_gpu_tools, igt_kms_flip_wf_vblank-vs-dpms-interruptible, DMESG_WARN(1, M26)PASS(15, M6M26M37) -> PASS(4, M6)
IVB: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(5, M4M34)PASS(11, M34M4) -> NSPT(2, M4)PASS(2, M4)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-A-plane-1, TIMEOUT(1, M34)PASS(9, M4) -> PASS(4, M4)
IVB: Intel_gpu_tools, igt_kms_flip_absolute-wf_vblank-interruptible, PASS(4, M34M4) -> DMESG_WARN(1, M4)PASS(3, M4)
SNB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-sliding, DMESG_WARN(3, M35M22)PASS(7, M22M35) -> PASS(4, M35)
SNB: Intel_gpu_tools, igt_kms_plane_plane-panning-top-left-pipe-A-plane-1, PASS(1, M35) -> FAIL(1, M35)PASS(3, M35)
SNB: Intel_gpu_tools, igt_pm_rpm_legacy-planes, TIMEOUT(1, M35) -> TIMEOUT(3, M35)PASS(1, M35)
SNB: Intel_gpu_tools, igt_pm_rpm_legacy-planes-dpms, TIMEOUT(1, M35) -> TIMEOUT(3, M35)PASS(1, M35)
SNB: Intel_gpu_tools, igt_pm_rpm_universal-planes, TIMEOUT(1, M35) -> TIMEOUT(2, M35)PASS(1, M35)
SNB: Intel_gpu_tools, igt_pm_rpm_universal-planes-dpms, TIMEOUT(1, M35) -> PASS(1, M35)
HSW: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(4, M39M20M40)PASS(9, M40M39M20) -> NSPT(2, M19)PASS(2, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_legacy-planes, TIMEOUT(1, M40) -> TIMEOUT(3, M19)PASS(1, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_legacy-planes-dpms, TIMEOUT(1, M40) -> TIMEOUT(3, M19)PASS(1, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_universal-planes, TIMEOUT(1, M40) -> TIMEOUT(2, M19)PASS(1, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_universal-planes-dpms, TIMEOUT(1, M40) -> PASS(1, M19)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, PASS(16, M30M28M42) -> DMESG_WARN(1, M30)PASS(3, M30)
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#16 lore raw

PATCH v4 3/5 drm/i915: use intel_fb_obj() macros to assign gem objects

In reply to Gustavo Padovan

From: Gustavo Padovan <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_sprite.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +3 -5
index 3631b0e..8b80d68 100644
@@ -1032,8 +1032,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
@@ -1235,9 +1234,8 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->fb;
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
Signature · 6 lines
1.9.3

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

PATCH v4 4/5 drm/i915: only flip frontbuffer if crtc is active

In reply to Gustavo Padovan

From: Gustavo Padovan <[email protected]>

There is no point in flipping a buffer for a disabled crtc.

---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_display.c +2 -2
index 8530401..9a913f5 100644
@@ -8544,9 +8544,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		if (old_width != width)
 			intel_update_watermarks(crtc);
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-	}
 
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+	}
 
 	return 0;
 fail_unpin:
Signature · 1 line
1.9.3
Reviewed-by on v4 4/5
#18 lore raw

Re: [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active

Quoted 3 lines · Gustavo Padovan

From: Gustavo Padovan <[email protected]>

There is no point in flipping a buffer for a disabled crtc.

That thing doesn't actually flip but just signal the frontbuffer tracking code that either has just flipped or is going to real soon now (tm). But yeah, still makes no sense when the entire pipe is off, so:

Quoted 29 lines
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_display.c +2 -2
index 8530401..9a913f5 100644
@@ -8544,9 +8544,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		if (old_width != width)
 			intel_update_watermarks(crtc);
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-	}
 
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+	}
 
 	return 0;
 fail_unpin:
Signature · 6 lines
1.9.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 2 lines
Ville Syrjälä
Intel OTC
#19 lore raw

Re: [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active

Quoted 10 lines · Ville Syrjälä
Quoted 3 lines · Gustavo Padovan

From: Gustavo Padovan <[email protected]>

There is no point in flipping a buffer for a disabled crtc.

That thing doesn't actually flip but just signal the frontbuffer tracking code that either has just flipped or is going to real soon now (tm). But yeah, still makes no sense when the entire pipe is off, so:

Queued for -next, thanks for the patch.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#20 lore raw

PATCH v4 5/5 drm/i915: remove intel_crtc_cursor_set_obj()

In reply to Gustavo Padovan

From: Gustavo Padovan <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
 1 file changed, 100 insertions(+), 115 deletions(-)
drivers/gpu/drm/i915/intel_display.c +100 -115
index 9a913f5..60ec165 100644
@@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 PTE of padding following
+		 * the bo. We currently fill all unused PTE with the shadow
+		 * page and so we should always have valid PTE following the
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, state->visible);
+
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
+
+	return 0;
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->base);
+	return ret;
 }
 
 static int
Signature · 1 line
1.9.3
#21 lore raw

Re: [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()

Quoted 239 lines · Gustavo Padovan

From: Gustavo Padovan <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
 1 file changed, 100 insertions(+), 115 deletions(-)
drivers/gpu/drm/i915/intel_display.c +80 -115
index 9a913f5..60ec165 100644
@@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 PTE of padding following
+		 * the bo. We currently fill all unused PTE with the shadow
+		 * page and so we should always have valid PTE following the
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;

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

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, state->visible);

So we need to make sure state->visible==false when there's no fb. I suppose we should just do that in drm_plane_helper_check_update().

Quoted 21 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
+
+	return 0;
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->base);
+	return ret;
 }
 
 static int
Signature · 6 lines
1.9.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä
Intel OTC
#22 lore raw

Re: [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()

Hi Ville,

2014-10-24 Ville Syrjälä <[email protected]>:

Quoted 245 lines · Gustavo Padovan
Quoted 244 lines

From: Gustavo Padovan <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
 1 file changed, 100 insertions(+), 115 deletions(-)
drivers/gpu/drm/i915/intel_display.c +85 -115
index 9a913f5..60ec165 100644
@@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 PTE of padding following
+		 * the bo. We currently fill all unused PTE with the shadow
+		 * page and so we should always have valid PTE following the
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
Quoted 8 lines

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

Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +5 -0
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, state->visible);

So we need to make sure state->visible==false when there's no fb. I suppose we should just do that in drm_plane_helper_check_update().

The code to check that is merged already, do you have any other comment on this patch?

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

Reviewed-by on v4 5/5
#23 lore raw

Re: [Intel-gfx] [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()

In reply to Gustavo Padovan
Quoted 249 lines · Gustavo Padovan

From: Gustavo Padovan <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
 1 file changed, 100 insertions(+), 115 deletions(-)
drivers/gpu/drm/i915/intel_display.c +90 -115
index 9a913f5..60ec165 100644
@@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 PTE of padding following
+		 * the bo. We currently fill all unused PTE with the shadow
+		 * page and so we should always have valid PTE following the
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, state->visible);

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

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +9 -0
+
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
+
+	return 0;
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->base);

That unref needs to be dropped. With that change the patch is

Quoted 11 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
+	return ret;
 }
 
 static int
Signature · 6 lines
1.9.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel