On Tue, Jan 27, 2015 at 09:43:00PM +0000, Chris Wilson wrote: > On Tue, Jan 27, 2015 at 04:09:37PM +0100, Daniel Vetter wrote: > > On Wed, Jan 14, 2015 at 11:20:56AM +0000, Chris Wilson wrote: > > > static int > > > i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, > > > struct intel_engine_cs *ring, > > > @@ -536,14 +589,21 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, > > > int ret; > > > > > > flags = 0; > > > - if (entry->flags & __EXEC_OBJECT_NEEDS_MAP) > > > - flags |= PIN_GLOBAL | PIN_MAPPABLE; > > > - if (entry->flags & EXEC_OBJECT_NEEDS_GTT) > > > - flags |= PIN_GLOBAL; > > > - if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS) > > > - flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS; > > > + if (!drm_mm_node_allocated(&vma->node)) { > > > + if (entry->flags & __EXEC_OBJECT_NEEDS_MAP) > > > + flags |= PIN_GLOBAL | PIN_MAPPABLE; > > > + if (entry->flags & EXEC_OBJECT_NEEDS_GTT) > > > + flags |= PIN_GLOBAL; > > > + if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS) > > > + flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS; > > > + } > > > > Hm, aren't we always calling reserve un buffers we know we've just > > unbound? Keeping the flags computation would at least be a good selfcheck > > about the consistency of our placing decisions, so I'd like to keep it. I still think this isn't required, even with the ping-pong preventer below kept. Maybe add a WARN_ON(drm_mm_node_allocated); just for paranoia instead? > > > > > > > > ret = i915_gem_object_pin(obj, vma->vm, entry->alignment, flags); > > > + if ((ret == -ENOSPC || ret == -E2BIG) && > > > + only_mappable_for_reloc(entry->flags)) > > > + ret = i915_gem_object_pin(obj, vma->vm, > > > + entry->alignment, > > > + flags & ~(PIN_GLOBAL | PIN_MAPPABLE)); > > > if (ret) > > > return ret; > > > > > > @@ -605,13 +665,13 @@ eb_vma_misplaced(struct i915_vma *vma) > > > vma->node.start & (entry->alignment - 1)) > > > return true; > > > > > > - if (entry->flags & __EXEC_OBJECT_NEEDS_MAP && !obj->map_and_fenceable) > > > - return true; > > > - > > > if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS && > > > vma->node.start < BATCH_OFFSET_BIAS) > > > return true; > > > > > > + if (entry->flags & __EXEC_OBJECT_NEEDS_MAP && !obj->map_and_fenceable) > > > + return !only_mappable_for_reloc(entry->flags); > > > > Hm, this seems to contradict your commit message a bit since it'll result > > in a behavioural change of what we try to push into mappable for relocs. > > > > Shouldn't we instead just clear the NEEDS_MAP flag in reserve_vma when the > > mappable pin fails and we fall back? > > This pair of chunks is required to prevent the ping-pong you just > described, which was very slow. Makes sense, but imo then requires a comment (e.g. "prevent costly ping-pong just for relocations") and some words in the commmit message. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx