Re: [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 13, 2013 at 06:09:07PM -0700, Ben Widawsky wrote:
> Cleanup the map and fenceable setting during bind to make more sense,
> and not check i915_is_ggtt() 2 unnecessary times
> 
> v2: Move the bools into the if block (Chris) - There are ways to tidy
> this function (fence calculations for instance) even further, but they
> are quite invasive, so I am punting on those unless specifically asked.
> 
> v3: Add newline between variable declaration and logic (Chris)
> 
> Recommended-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4a58ead..01cc016 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  	struct drm_device *dev = obj->base.dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u32 size, fence_size, fence_alignment, unfenced_alignment;
> -	bool mappable, fenceable;
>  	size_t gtt_max =
>  		map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;
>  	struct i915_vma *vma;
> @@ -3191,18 +3190,18 @@ search_free:
>  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
>  	list_add_tail(&vma->mm_list, &vm->inactive_list);
>  
> -	fenceable =
> -		i915_is_ggtt(vm) &&
> -		i915_gem_obj_ggtt_size(obj) == fence_size &&
> -		(i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;
> +	if (i915_is_ggtt(vm)) {
> +		bool mappable, fenceable;
>  
> -	mappable =
> -		i915_is_ggtt(vm) &&
> -		vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end;
> +		fenceable =
> +			i915_gem_obj_ggtt_size(obj) == fence_size &&
> +			(i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;

Why not go the full mounty and also use vma->node here? Would also make
checkpatch a bit more happy. I'll do a follow-up commit.
-Daniel

> +
> +		mappable =
> +			vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end;
>  
> -	/* Map and fenceable only changes if the VM is the global GGTT */
> -	if (i915_is_ggtt(vm))
>  		obj->map_and_fenceable = mappable && fenceable;
> +	}
>  
>  	WARN_ON(map_and_fenceable && !obj->map_and_fenceable);
>  
> -- 
> 1.8.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch