Re: [PATCH] drm/i915: Consolidate binding parameters into flags

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 28 Jan 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Anything more than just one bool parameter is just a pain to read,
> symbolic constants are much better.
>
> Split out from Chris' vma-binding rework patch.
>
> v2: Undo the behaviour change in object_pin that Chris spotted.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

[snip]

> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 032def901f98..9399a6fa4c2f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -544,19 +544,23 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
>  	bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4;
> -	bool need_fence, need_mappable;
> -	u32 flags = (entry->flags & EXEC_OBJECT_NEEDS_GTT) &&
> -		!vma->obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
> +	bool need_fence;
> +	unsigned flags;
>  	int ret;
>  
> +	flags = 0;
> +
>  	need_fence =
>  		has_fenced_gpu_access &&
>  		entry->flags & EXEC_OBJECT_NEEDS_FENCE &&
>  		obj->tiling_mode != I915_TILING_NONE;
> -	need_mappable = need_fence || need_reloc_mappable(vma);
> +	if (need_fence || need_reloc_mappable(vma))
> +		flags |= PIN_MAPPABLE;
> +
> +	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
> +		flags |= PIN_GLOBAL;

It is not obvious to me that this together with the PIN_GLOBAL handling
in i915_gem_object_pin() do not introduce a functional change. (Stress
on obvious to _me_; it may be obvious to you.)

I would have thought it better to first change the two bool parameters
to two flags, and then add the new flag in a separate patch to not
confuse poor reviewers like myself.

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d897a19f887f..a0793c929b95 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -531,9 +531,11 @@ init_pipe_control(struct intel_ring_buffer *ring)
>  		goto err;
>  	}
>  
> -	i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC);
> +	ret = i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC);
> +	if (ret)
> +		goto err_unref;

This should be split out from the patch just like patch 4/9.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center