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

Jani Nikula
2014-01-28T13:06:13Z

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