Re: [Intel-gfx] [PATCH 3/3] drm/i915: Avoid unbinding due to an interrupted pin_and_fence during execbuffer
On Thu, Aug 23, 2012 at 01:12:53PM +0100, Chris Wilson wrote:
If we need to stall in order to complete the pin_and_fence operation
during execbuffer reservation, there is a high likelihood that the
operation will be interrupted by a signal (thanks X!). In order to
simplify the cleanup along that error path, the object was
unconditionally unbound and the error propagated. However, being
interrupted here is far more common than I would like and so we can
strive to avoid the extra work by eliminating the forced unbind.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
I've merged the resend of patch 1/3, thanks a lot. But for this one here
I've found a few tiny things to bitch about. Comments inline below.
-Daniel
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 93
1 file changed, 38 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 23aa324..0c5a433 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -331,7 +331,8 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
return ret;
}
-#define __EXEC_OBJECT_HAS_FENCE (131)
+#define __EXEC_OBJECT_HAS_PIN (131)
+#define __EXEC_OBJECT_HAS_FENCE (130)
static int
need_reloc_mappable(struct drm_i915_gem_object *obj)
@@ -344,6 +345,7 @@ static int
pin_and_fence_object(struct drm_i915_gem_object *obj,
struct intel_ring_buffer *ring)
{
+ struct drm_i915_private *dev_priv = obj-base.dev-dev_private;
struct drm_i915_gem_exec_object2 *entry = obj-exec_entry;
bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4;
bool need_fence, need_mappable;
@@ -359,11 +361,13 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
if (ret)
return ret;
+ entry-flags |= __EXEC_OBJECT_HAS_PIN;
+
if (has_fenced_gpu_access) {
if (entry-flags EXEC_OBJECT_NEEDS_FENCE) {
ret = i915_gem_object_get_fence(obj);
if (ret)
- goto err_unpin;
+ return ret;
if (i915_gem_object_pin_fence(obj))
entry-flags |= __EXEC_OBJECT_HAS_FENCE;
@@ -372,12 +376,35 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
}
}
+ /* ... and ensure ppgtt mapping exist if needed. */
Nitpick: the ... and in this move comment looks a bit stale with the
previously preceeding comment no longer in the same scope ;-)
+ if (dev_priv-mm.aliasing_ppgtt !obj-has_aliasing_ppgtt_mapping) {
+ i915_ppgtt_bind_object(dev_priv-mm.aliasing_ppgtt,
+obj, obj-cache_level);
+
+ obj-has_aliasing_ppgtt_mapping = 1;
+ }
+
entry-offset = obj-gtt_offset;
return 0;
+}
-err_unpin:
- i915_gem_object_unpin(obj);
- return ret;
+static void
+unpin_and_fence_object(struct drm_i915_gem_object *obj)
+{
+ struct drm_i915_gem_exec_object2 *entry;
+
+ if (!obj-gtt_space)
+ return;
+
+ entry = obj-exec_entry;
+
+ if (entry-flags __EXEC_OBJECT_HAS_FENCE)
+ i915_gem_object_unpin_fence(obj);
+
+ if (entry-flags __EXEC_OBJECT_HAS_PIN)
+ i915_gem_object_unpin(obj);
+
+ entry-flags = ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
}
static int
@@ -385,7 +412,6 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer
*ring,
struct drm_file *file,
struct list_head *objects)
{
- drm_i915_private_t *dev_priv = ring-dev-dev_private;
struct drm_i915_gem_object *obj;
int ret, retry;
bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4;
@@ -463,45 +489,13 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer
*ring,
continue;
ret = pin_and_fence_object(obj, ring);
- if (ret) {
- int ret_ignore;
-
- /* This can potentially raise a harmless
- * -EINVAL if we failed to bind in the above
- * call. It cannot raise -EINTR since we know
- * that the bo is freshly bound and so will
- * not need to be flushed or waited upon.
- */
- ret_ignore = i915_gem_object_unbind(obj);
- (void)ret_ignore;
- WARN_ON(obj-gtt_space);
+ if (ret)
break;
- }
}
/* Decrement pin count for bound objects */
-
[Intel-gfx] [PATCH 3/3] drm/i915: Avoid unbinding due to an interrupted pin_and_fence during execbuffer
If we need to stall in order to complete the pin_and_fence operation
during execbuffer reservation, there is a high likelihood that the
operation will be interrupted by a signal (thanks X!). In order to
simplify the cleanup along that error path, the object was
unconditionally unbound and the error propagated. However, being
interrupted here is far more common than I would like and so we can
strive to avoid the extra work by eliminating the forced unbind.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 93
1 file changed, 38 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 23aa324..0c5a433 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -331,7 +331,8 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
return ret;
}
-#define __EXEC_OBJECT_HAS_FENCE (131)
+#define __EXEC_OBJECT_HAS_PIN (131)
+#define __EXEC_OBJECT_HAS_FENCE (130)
static int
need_reloc_mappable(struct drm_i915_gem_object *obj)
@@ -344,6 +345,7 @@ static int
pin_and_fence_object(struct drm_i915_gem_object *obj,
struct intel_ring_buffer *ring)
{
+ struct drm_i915_private *dev_priv = obj-base.dev-dev_private;
struct drm_i915_gem_exec_object2 *entry = obj-exec_entry;
bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4;
bool need_fence, need_mappable;
@@ -359,11 +361,13 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
if (ret)
return ret;
+ entry-flags |= __EXEC_OBJECT_HAS_PIN;
+
if (has_fenced_gpu_access) {
if (entry-flags EXEC_OBJECT_NEEDS_FENCE) {
ret = i915_gem_object_get_fence(obj);
if (ret)
- goto err_unpin;
+ return ret;
if (i915_gem_object_pin_fence(obj))
entry-flags |= __EXEC_OBJECT_HAS_FENCE;
@@ -372,12 +376,35 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
}
}
+ /* ... and ensure ppgtt mapping exist if needed. */
+ if (dev_priv-mm.aliasing_ppgtt !obj-has_aliasing_ppgtt_mapping) {
+ i915_ppgtt_bind_object(dev_priv-mm.aliasing_ppgtt,
+ obj, obj-cache_level);
+
+ obj-has_aliasing_ppgtt_mapping = 1;
+ }
+
entry-offset = obj-gtt_offset;
return 0;
+}
-err_unpin:
- i915_gem_object_unpin(obj);
- return ret;
+static void
+unpin_and_fence_object(struct drm_i915_gem_object *obj)
+{
+ struct drm_i915_gem_exec_object2 *entry;
+
+ if (!obj-gtt_space)
+ return;
+
+ entry = obj-exec_entry;
+
+ if (entry-flags __EXEC_OBJECT_HAS_FENCE)
+ i915_gem_object_unpin_fence(obj);
+
+ if (entry-flags __EXEC_OBJECT_HAS_PIN)
+ i915_gem_object_unpin(obj);
+
+ entry-flags = ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
}
static int
@@ -385,7 +412,6 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
struct drm_file *file,
struct list_head *objects)
{
- drm_i915_private_t *dev_priv = ring-dev-dev_private;
struct drm_i915_gem_object *obj;
int ret, retry;
bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4;
@@ -463,45 +489,13 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer
*ring,
continue;
ret = pin_and_fence_object(obj, ring);
- if (ret) {
- int ret_ignore;
-
- /* This can potentially raise a harmless
-* -EINVAL if we failed to bind in the above
-* call. It cannot raise -EINTR since we know
-* that the bo is freshly bound and so will
-* not need to be flushed or waited upon.
-*/
- ret_ignore = i915_gem_object_unbind(obj);
- (void)ret_ignore;
- WARN_ON(obj-gtt_space);
+ if (ret)
break;
- }
}
/* Decrement pin count for bound objects */
- list_for_each_entry(obj, objects, exec_list) {
- struct drm_i915_gem_exec_object2 *entry;
-
- if (!obj-gtt_space)
- continue;
-
- entry = obj-exec_entry;
- if (entry-flags __EXEC_OBJECT_HAS_FENCE) {
-
