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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
