Re: [Intel-gfx] [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing
On Fri, Mar 18, 2011 at 08:02:10AM +, Chris Wilson wrote:
... even though it was disabled. A mistake in the handling of fence reuse
caused us to skip the vital delay of waiting for the object to finish
rendering before changing the register.
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Nice catch and good simplification of the code-flow. One nitpick about a
possible further cleanup below.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d4bf061..c5dfb59 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj,
reg = dev_priv-fence_regs[obj-fence_reg];
list_move_tail(reg-lru_list, dev_priv-mm.fence_list);
- if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
- pipelined = NULL;
+ if (obj-tiling_changed) {
+ ret = i915_gem_object_flush_fence(obj, pipelined);
+ if (ret)
+ return ret;
+
+ if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
+ pipelined = NULL;
+
+ if (pipelined) {
+ reg-setup_seqno =
+ i915_gem_next_request_seqno(pipelined);
+ obj-last_fenced_seqno = reg-setup_seqno;
+ obj-last_fenced_ring = pipelined;
+ }
+
+ goto update;
I think we could move the update label 3 lines up, which would make the
above if(pipelined) clause unnecessary. Maybe even drop the goto and
extract the tail of get_fence as a function of its own.
+ }
if (!pipelined) {
if (reg-setup_seqno) {
@@ -2601,31 +2616,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj,
ret = i915_gem_object_flush_fence(obj, pipelined);
if (ret)
return ret;
- } else if (obj-tiling_changed) {
- if (obj-fenced_gpu_access) {
- if (obj-base.write_domain
I915_GEM_GPU_DOMAINS) {
- ret = i915_gem_flush_ring(obj-ring,
- 0,
obj-base.write_domain);
- if (ret)
- return ret;
- }
-
- obj-fenced_gpu_access = false;
- }
- }
-
- if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
- pipelined = NULL;
- BUG_ON(!pipelined reg-setup_seqno);
-
- if (obj-tiling_changed) {
- if (pipelined) {
- reg-setup_seqno =
- i915_gem_next_request_seqno(pipelined);
- obj-last_fenced_seqno = reg-setup_seqno;
- obj-last_fenced_ring = pipelined;
- }
- goto update;
}
return 0;
--
1.7.2.3
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing
On Sat, 19 Mar 2011 23:17:56 +0100, Daniel Vetter dan...@ffwll.ch wrote:
On Fri, Mar 18, 2011 at 08:02:10AM +, Chris Wilson wrote:
... even though it was disabled. A mistake in the handling of fence reuse
caused us to skip the vital delay of waiting for the object to finish
rendering before changing the register.
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Nice catch and good simplification of the code-flow. One nitpick about a
possible further cleanup below.
Hints for further cleanups are always appreciated. :)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index d4bf061..c5dfb59 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj,
reg = dev_priv-fence_regs[obj-fence_reg];
list_move_tail(reg-lru_list, dev_priv-mm.fence_list);
- if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
- pipelined = NULL;
+ if (obj-tiling_changed) {
+ ret = i915_gem_object_flush_fence(obj, pipelined);
+ if (ret)
+ return ret;
+
+ if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
+ pipelined = NULL;
+
+ if (pipelined) {
+ reg-setup_seqno =
+ i915_gem_next_request_seqno(pipelined);
+ obj-last_fenced_seqno = reg-setup_seqno;
+ obj-last_fenced_ring = pipelined;
+ }
+
+ goto update;
I think we could move the update label 3 lines up, which would make the
above if(pipelined) clause unnecessary. Maybe even drop the goto and
extract the tail of get_fence as a function of its own.
Right, I did this in a later patch. I want to keep this as a simple
rearrangement of the code since that is the minimal fix and improve upon
that with further patches. Sound good?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing
... even though it was disabled. A mistake in the handling of fence reuse
caused us to skip the vital delay of waiting for the object to finish
rendering before changing the register.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34584
Cc: Andy Whitcroft a...@canonical.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
[Note for 2.6.38-stable, we need to reintroduce the interruptible passing]
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem.c | 44 +++---
1 files changed, 17 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d4bf061..c5dfb59 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj,
reg = dev_priv-fence_regs[obj-fence_reg];
list_move_tail(reg-lru_list, dev_priv-mm.fence_list);
- if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
- pipelined = NULL;
+ if (obj-tiling_changed) {
+ ret = i915_gem_object_flush_fence(obj, pipelined);
+ if (ret)
+ return ret;
+
+ if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
+ pipelined = NULL;
+
+ if (pipelined) {
+ reg-setup_seqno =
+ i915_gem_next_request_seqno(pipelined);
+ obj-last_fenced_seqno = reg-setup_seqno;
+ obj-last_fenced_ring = pipelined;
+ }
+
+ goto update;
+ }
if (!pipelined) {
if (reg-setup_seqno) {
@@ -2601,31 +2616,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj,
ret = i915_gem_object_flush_fence(obj, pipelined);
if (ret)
return ret;
- } else if (obj-tiling_changed) {
- if (obj-fenced_gpu_access) {
- if (obj-base.write_domain
I915_GEM_GPU_DOMAINS) {
- ret = i915_gem_flush_ring(obj-ring,
- 0,
obj-base.write_domain);
- if (ret)
- return ret;
- }
-
- obj-fenced_gpu_access = false;
- }
- }
-
- if (!obj-fenced_gpu_access !obj-last_fenced_seqno)
- pipelined = NULL;
- BUG_ON(!pipelined reg-setup_seqno);
-
- if (obj-tiling_changed) {
- if (pipelined) {
- reg-setup_seqno =
- i915_gem_next_request_seqno(pipelined);
- obj-last_fenced_seqno = reg-setup_seqno;
- obj-last_fenced_ring = pipelined;
- }
- goto update;
}
return 0;
--
1.7.2.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
