| Message ID | 1345723973-22092-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <chris@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 (1<<31) > +#define __EXEC_OBJECT_HAS_PIN (1<<31) > +#define __EXEC_OBJECT_HAS_FENCE (1<<30) > > 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 */ > - 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) { > - i915_gem_object_unpin_fence(obj); > - entry->flags &= ~__EXEC_OBJECT_HAS_FENCE; > - } > - > - i915_gem_object_unpin(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; > - } > - } > + list_for_each_entry(obj, objects, exec_list) > + unpin_and_fence_object(obj); > > if (ret != -ENOSPC || retry++) > return ret; > @@ -512,20 +506,9 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, > } while (1); > > err: > - list_for_each_entry_continue_reverse(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) { > - i915_gem_object_unpin_fence(obj); > - entry->flags &= ~__EXEC_OBJECT_HAS_FENCE; > - } > - > - i915_gem_object_unpin(obj); > - } > + do { > + unpin_and_fence_object(obj); > + } while (&(obj = list_entry(obj->exec_list.prev, typeof(*obj), exec_list))->exec_list != objects); What's the reason here for no longer using the continue_reverse macro? On a quick glance the new thing seems to do the same ... > > return ret; > } > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 24 Aug 2012 16:46:19 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > 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 <chris@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 (1<<31) > > +#define __EXEC_OBJECT_HAS_PIN (1<<31) > > +#define __EXEC_OBJECT_HAS_FENCE (1<<30) > > > > 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 ;-) Hey, I thought you were just adding a bit of drama! > > - list_for_each_entry_continue_reverse(obj, objects, exec_list) { > > + do { > > + unpin_and_fence_object(obj); > > + } while (&(obj = list_entry(obj->exec_list.prev, typeof(*obj), exec_list))->exec_list != objects); > > What's the reason here for no longer using the continue_reverse macro? On > a quick glance the new thing seems to do the same ... Because this is actually list_for_each_entry_from_reverse(), which doesn't yet exist. We used to unbind the current obj on error so that we could walk backwards along the list, but now we need to start the reverse walk from the error object. -Chris
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 (1<<31) +#define __EXEC_OBJECT_HAS_PIN (1<<31) +#define __EXEC_OBJECT_HAS_FENCE (1<<30) 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) { - i915_gem_object_unpin_fence(obj); - entry->flags &= ~__EXEC_OBJECT_HAS_FENCE; - } - - i915_gem_object_unpin(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; - } - } + list_for_each_entry(obj, objects, exec_list) + unpin_and_fence_object(obj); if (ret != -ENOSPC || retry++) return ret; @@ -512,20 +506,9 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, } while (1); err: - list_for_each_entry_continue_reverse(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) { - i915_gem_object_unpin_fence(obj); - entry->flags &= ~__EXEC_OBJECT_HAS_FENCE; - } - - i915_gem_object_unpin(obj); - } + do { + unpin_and_fence_object(obj); + } while (&(obj = list_entry(obj->exec_list.prev, typeof(*obj), exec_list))->exec_list != objects); return ret; }
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 <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 93 ++++++++++++---------------- 1 file changed, 38 insertions(+), 55 deletions(-)