Re: [PATCH] drm/i915: simplify bind_to_vm init code

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, Matthew Auld <matthew.auld@intel.com>
Subject: Re: [PATCH] drm/i915: simplify bind_to_vm init code
Date: Fri, 18 Mar 2016 11:26:31 +0000	[thread overview]
Message-ID: <20160318112631.GG21717@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <56EBE23A.5020506@linux.intel.com>

On Fri, Mar 18, 2016 at 11:10:50AM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 18/03/16 10:46, Matthew Auld wrote:
> >No functional change, just makes the code easier to follow.
> >
> >v2:
> >     - Remove local fence_size variable
> >(Tvrtko Ursulin)
> >     - Remove redundant NULL ggtt_view check
> >     - Reuse size variable
> >
> >Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------
> >  1 file changed, 14 insertions(+), 37 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index f45856d..e5d9d0b 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  	u32 fence_alignment, unfenced_alignment;
> >  	u32 search_flag, alloc_flag;
> >  	u64 start, end;
> >-	u64 size, fence_size;
> >+	u64 size;
> >  	struct i915_vma *vma;
> >  	int ret;
> >
> >-	if (i915_is_ggtt(vm)) {
> >-		u32 view_size;
> >-
> >-		if (WARN_ON(!ggtt_view))
> >-			return ERR_PTR(-EINVAL);
> >-
> >-		view_size = i915_ggtt_view_size(obj, ggtt_view);
> >-
> >-		fence_size = i915_gem_get_gtt_size(dev,
> >-						   view_size,
> >-						   obj->tiling_mode);
> >-		fence_alignment = i915_gem_get_gtt_alignment(dev,
> >-							     view_size,
> >-							     obj->tiling_mode,
> >-							     true);
> >-		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> >-								view_size,
> >-								obj->tiling_mode,
> >-								false);
> >-		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> >-	} else {
> >-		fence_size = i915_gem_get_gtt_size(dev,
> >-						   obj->base.size,
> >-						   obj->tiling_mode);
> >-		fence_alignment = i915_gem_get_gtt_alignment(dev,
> >-							     obj->base.size,
> >-							     obj->tiling_mode,
> >-							     true);
> >-		unfenced_alignment =
> >-			i915_gem_get_gtt_alignment(dev,
> >-						   obj->base.size,
> >-						   obj->tiling_mode,
> >-						   false);
> >-		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> >-	}
> >+	if (i915_is_ggtt(vm))
> >+		size = i915_ggtt_view_size(obj, ggtt_view);
> >+	else
> >+		size = obj->base.size;
> >+
> >+	fence_alignment = i915_gem_get_gtt_alignment(dev, size,
> >+						     obj->tiling_mode, true);
> >+	unfenced_alignment = i915_gem_get_gtt_alignment(dev, size,
> >+							obj->tiling_mode,
> >+							false);
> >
> >  	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> >  	end = vm->total;
> >-	if (flags & PIN_MAPPABLE)
> >+	if (flags & PIN_MAPPABLE) {
> >  		end = min_t(u64, end, dev_priv->gtt.mappable_end);
> >+		size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode);

No. Keep the start, end computation separate.

For example the above size needs only be done when inspecting the
i915_is_ggtt().

If you simplified the alignement as well, it becomes clearer. If you
reviewed the patches to handle vma, it would help. The key question is
what igt did you run to verify the changes?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx