drm/i915: create a prepare step for primary planes updates
Subsystems (2)
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
@@ -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
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
@@ -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
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_fbcode 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
Signed-off-by Gustavo Padovan <[email protected]>Reviewed-by Ville Syrjälä <[email protected]>--- drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-)drivers/gpu/drm/i915/intel_sprite.c +40
-24@@ -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
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_fbcode 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
Signed-off-by Gustavo Padovan <[email protected]>Reviewed-by Ville Syrjälä <[email protected]>--- drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-)drivers/gpu/drm/i915/intel_sprite.c +40
-23@@ -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
-1static 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
Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates
In reply to Paulo ZanoniQuoted 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_fbcode 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
Signed-off-by Gustavo Padovan <[email protected]>Reviewed-by Ville Syrjälä <[email protected]>--- drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-)drivers/gpu/drm/i915/intel_sprite.c +2
-2@@ -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-gfxSignature · 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
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]>
--- drivers/gpu/drm/i915/intel_sprite.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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;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
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_objpointer we use when committing sprite planes, which caused aWARN()and aBUG()to be triggered. This patch should revert the code back to the previous behavior, fixing the regression.Regression introduced by:
commitec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updatesCredits 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]>Signed-off-by Paulo Zanoni <[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
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
-0int 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
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_objpointer we use when committing sprite planes, which caused aWARN()and aBUG()to be triggered. This patch should revert the code back to the previous behavior, fixing the regression.Regression introduced by:
commitec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updatesCredits 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]>Signed-off-by Paulo Zanoni <[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
-0drivers/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=A2. .crtc_disable()
->disable()-> unpin(A) -> obj=NULL3. .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
-1int 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
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]>
--- drivers/gpu/drm/i915/intel_sprite.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +3 -2
@@ -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
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_objpointer we use when committing sprite planes, which caused aWARN()and aBUG()to be triggered. Later, commit "drm/i915: useintel_fb_obj()macros to assign gem objects" introduced the same problem to functionintel_commit_sprite_plane().Regression introduced by:
commitec82cb793c9224e0692eed904f43490cf70e8258
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:
commit77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:33 2014 +0100
drm/i915: useintel_fb_obj()macros to assign gem objectsCredits 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]>Signed-off-by Paulo Zanoni <[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@@ -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
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_disabletheme.
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
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_disabletheme.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
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_disabletheme.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->fbalone 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->fbis 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
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_disabletheme.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->fbalone 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->fbis 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_configatomic 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
Daniel Vetter
AppliedRe: [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_objpointer we use when committing sprite planes, which caused aWARN()and aBUG()to be triggered. Later, commit "drm/i915: useintel_fb_obj()macros to assign gem objects" introduced the same problem to functionintel_commit_sprite_plane().Regression introduced by:
commitec82cb793c9224e0692eed904f43490cf70e8258
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:
commit77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan <[email protected]>
Date: Fri Oct 24 14:51:33 2014 +0100
drm/i915: useintel_fb_obj()macros to assign gem objectsCredits 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]>Signed-off-by Paulo Zanoni <[email protected]>
Quoted 3 lines
Yep, I believe that should do it.
Reviewed-by Ville Syrjälä <[email protected]>
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
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
PATCH v4 3/5 drm/i915: use intel_fb_obj() macros to assign gem objects
In reply to Gustavo PadovanFrom: 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
@@ -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
PATCH v4 4/5 drm/i915: only flip frontbuffer if crtc is active
In reply to Gustavo PadovanFrom: 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
@@ -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
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
Signed-off-by Gustavo Padovan <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_display.c +2
-2@@ -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
Daniel Vetter
AppliedRe: [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:
Reviewed-by Ville Syrjälä <[email protected]>
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
PATCH v4 5/5 drm/i915: remove intel_crtc_cursor_set_obj()
In reply to Gustavo PadovanFrom: 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
@@ -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
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 theupdate_plane()functions instead.The fb !=
crtc->cursor->fbwas already insideintel_crtc_cursor_set_obj()so we foldintel_crtc_cursor_set_obj()insideintel_commit_cursor_plane()and merge both paths into one.Signed-off-by Gustavo Padovan <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++------------------- 1 file changed, 100 insertions(+), 115 deletions(-)drivers/gpu/drm/i915/intel_display.c +80
-115@@ -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 intSignature · 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
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 theupdate_plane()functions instead.The fb !=
crtc->cursor->fbwas already insideintel_crtc_cursor_set_obj()so we foldintel_crtc_cursor_set_obj()insideintel_commit_cursor_plane()and merge both paths into one.Signed-off-by Gustavo Padovan <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++------------------- 1 file changed, 100 insertions(+), 115 deletions(-)drivers/gpu/drm/i915/intel_display.c +85
-115@@ -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
Re: [Intel-gfx] [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()
In reply to Gustavo PadovanQuoted 249 lines · Gustavo Padovan
From: Gustavo Padovan <[email protected]>
Merge it into the plane
update_plane()callback and make other users use theupdate_plane()functions instead.The fb !=
crtc->cursor->fbwas already insideintel_crtc_cursor_set_obj()so we foldintel_crtc_cursor_set_obj()insideintel_commit_cursor_plane()and merge both paths into one.Signed-off-by Gustavo Padovan <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++------------------- 1 file changed, 100 insertions(+), 115 deletions(-)drivers/gpu/drm/i915/intel_display.c +90
-115@@ -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 intSignature · 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