Re: [PATCH 03/20] drm/i915/gt: Provde a local intel_context.vm

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/07/2019 08:00, Chris Wilson wrote:
> Track the currently bound address space used by the HW context. Minor
> conversions to use the local intel_context.vm are made, leaving behind
> some more surgery required to make intel_context the primary through the
> selftests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_client_blt.c |  4 +---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c    | 13 ++++++++++---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 +++--------
>   drivers/gpu/drm/i915/gem/i915_gem_object_blt.c |  6 +-----
>   drivers/gpu/drm/i915/gt/intel_context.c        |  4 ++++
>   drivers/gpu/drm/i915/gt/intel_context_types.h  |  4 +++-
>   drivers/gpu/drm/i915/gt/intel_ringbuffer.c     |  6 +++---
>   drivers/gpu/drm/i915/i915_gpu_error.c          |  2 +-
>   8 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> index 6f537e8e4dea..2312a0c6af89 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> @@ -250,13 +250,11 @@ int i915_gem_schedule_fill_pages_blt(struct drm_i915_gem_object *obj,
>   				     u32 value)
>   {
>   	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> -	struct i915_gem_context *ctx = ce->gem_context;
> -	struct i915_address_space *vm = ctx->vm ?: &i915->ggtt.vm;
>   	struct clear_pages_work *work;
>   	struct i915_sleeve *sleeve;
>   	int err;
>   
> -	sleeve = create_sleeve(vm, obj, pages, page_sizes);
> +	sleeve = create_sleeve(ce->vm, obj, pages, page_sizes);
>   	if (IS_ERR(sleeve))
>   		return PTR_ERR(sleeve);
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 0f6b0678f548..de9bb7aaef22 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -475,10 +475,18 @@ static struct i915_address_space *
>   __set_ppgtt(struct i915_gem_context *ctx, struct i915_address_space *vm)
>   {
>   	struct i915_address_space *old = ctx->vm;
> +	struct i915_gem_engines_iter it;
> +	struct intel_context *ce;
>   
>   	ctx->vm = i915_vm_get(vm);
>   	ctx->desc_template = default_desc_template(ctx->i915, vm);
>   
> +	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> +		i915_vm_put(ce->vm);
> +		ce->vm = i915_vm_get(vm);
> +	}
> +	i915_gem_context_unlock_engines(ctx);
> +
>   	return old;
>   }
>   
> @@ -1113,9 +1121,8 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
>   				   set_ppgtt_barrier,
>   				   old);
>   	if (err) {
> -		ctx->vm = old;
> -		ctx->desc_template = default_desc_template(ctx->i915, old);
> -		i915_vm_put(vm);
> +		i915_vm_put(__set_ppgtt(ctx, old));
> +		i915_vm_put(old);

Shouldn't this still be i915_vm_out(vm), for the extra vm reference the 
function did further up?

>   	}
>   
>   unlock:
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 8a2047c4e7c3..cbd7c6e3a1f8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -223,7 +223,6 @@ struct i915_execbuffer {
>   	struct intel_engine_cs *engine; /** engine to queue the request to */
>   	struct intel_context *context; /* logical state for the request */
>   	struct i915_gem_context *gem_context; /** caller's context */
> -	struct i915_address_space *vm; /** GTT and vma for the request */
>   
>   	struct i915_request *request; /** our request to build */
>   	struct i915_vma *batch; /** identity of the batch obj/vma */
> @@ -697,7 +696,7 @@ static int eb_reserve(struct i915_execbuffer *eb)
>   
>   		case 1:
>   			/* Too fragmented, unbind everything and retry */
> -			err = i915_gem_evict_vm(eb->vm);
> +			err = i915_gem_evict_vm(eb->context->vm);
>   			if (err)
>   				return err;
>   			break;
> @@ -725,12 +724,8 @@ static int eb_select_context(struct i915_execbuffer *eb)
>   		return -ENOENT;
>   
>   	eb->gem_context = ctx;
> -	if (ctx->vm) {
> -		eb->vm = ctx->vm;
> +	if (ctx->vm)
>   		eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
> -	} else {
> -		eb->vm = &eb->i915->ggtt.vm;
> -	}
>   
>   	eb->context_flags = 0;
>   	if (test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags))
> @@ -832,7 +827,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
>   			goto err_vma;
>   		}
>   
> -		vma = i915_vma_instance(obj, eb->vm, NULL);
> +		vma = i915_vma_instance(obj, eb->context->vm, NULL);
>   		if (IS_ERR(vma)) {
>   			err = PTR_ERR(vma);
>   			goto err_obj;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> index cb42e3a312e2..685064af32d1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> @@ -47,15 +47,11 @@ int i915_gem_object_fill_blt(struct drm_i915_gem_object *obj,
>   			     struct intel_context *ce,
>   			     u32 value)
>   {
> -	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> -	struct i915_gem_context *ctx = ce->gem_context;
> -	struct i915_address_space *vm = ctx->vm ?: &i915->ggtt.vm;
>   	struct i915_request *rq;
>   	struct i915_vma *vma;
>   	int err;
>   
> -	/* XXX: ce->vm please */
> -	vma = i915_vma_instance(obj, vm, NULL);
> +	vma = i915_vma_instance(obj, ce->vm, NULL);
>   	if (IS_ERR(vma))
>   		return PTR_ERR(vma);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 9292b6ca5e9c..9e4f51ce52ff 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -191,6 +191,8 @@ intel_context_init(struct intel_context *ce,
>   	kref_init(&ce->ref);
>   
>   	ce->gem_context = ctx;
> +	ce->vm = i915_vm_get(ctx->vm ?: &engine->gt->ggtt->vm);
> +
>   	ce->engine = engine;
>   	ce->ops = engine->cops;
>   	ce->sseu = engine->sseu;
> @@ -206,6 +208,8 @@ intel_context_init(struct intel_context *ce,
>   
>   void intel_context_fini(struct intel_context *ce)
>   {
> +	i915_vm_put(ce->vm);
> +
>   	mutex_destroy(&ce->pin_mutex);
>   	i915_active_fini(&ce->active);
>   }
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 4c0e211c715d..68a7e979b1a9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -36,7 +36,6 @@ struct intel_context_ops {
>   struct intel_context {
>   	struct kref ref;
>   
> -	struct i915_gem_context *gem_context;
>   	struct intel_engine_cs *engine;
>   	struct intel_engine_cs *inflight;
>   #define intel_context_inflight(ce) ptr_mask_bits((ce)->inflight, 2)
> @@ -44,6 +43,9 @@ struct intel_context {
>   #define intel_context_inflight_inc(ce) ptr_count_inc(&(ce)->inflight)
>   #define intel_context_inflight_dec(ce) ptr_count_dec(&(ce)->inflight)
>   
> +	struct i915_address_space *vm;
> +	struct i915_gem_context *gem_context;
> +
>   	struct list_head signal_link;
>   	struct list_head signals;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index b056f25c66f2..38ec11ae6ed7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -1396,9 +1396,9 @@ static struct i915_address_space *vm_alias(struct intel_context *ce)
>   {
>   	struct i915_address_space *vm;
>   
> -	vm = ce->gem_context->vm;
> -	if (!vm)
> -		vm = &ce->engine->gt->ggtt->alias->vm;
> +	vm = ce->vm;
> +	if (i915_is_ggtt(vm))
> +		vm = &i915_vm_to_ggtt(vm)->alias->vm;
>   
>   	return vm;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c5b89bf4d616..24835be300bc 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1429,7 +1429,7 @@ static void gem_record_rings(struct i915_gpu_state *error)
>   			struct i915_gem_context *ctx = request->gem_context;
>   			struct intel_ring *ring = request->ring;
>   
> -			ee->vm = ctx->vm ?: &engine->gt->ggtt->vm;
> +			ee->vm = request->hw_context->vm;
>   
>   			record_context(&ee->context, ctx);
>   
> 

Rest looks fine.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx