[03/20] drm/i915/gt: Provde a local intel_context.vm
diff mbox series

Message ID 20190718070024.21781-3-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/20] drm/i915: Move aliasing_ppgtt underneath its i915_ggtt
Related show

Commit Message

Chris Wilson July 18, 2019, 7 a.m. UTC
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(-)

Comments

Tvrtko Ursulin July 22, 2019, 12:33 p.m. UTC | #1
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
Chris Wilson July 22, 2019, 4:28 p.m. UTC | #2
Quoting Tvrtko Ursulin (2019-07-22 13:33:14)
> 
> On 18/07/2019 08:00, Chris Wilson wrote:
> > @@ -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?

__set_ppgtt() returns the vm, so this is
	i915_vm_put(vm);
	i915_vm_put(old); /* since we just acquired a ref to old again */
-Chris
Tvrtko Ursulin July 23, 2019, 1:44 p.m. UTC | #3
On 22/07/2019 17:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-22 13:33:14)
>>
>> On 18/07/2019 08:00, Chris Wilson wrote:
>>> @@ -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?
> 
> __set_ppgtt() returns the vm, so this is
> 	i915_vm_put(vm);
> 	i915_vm_put(old); /* since we just acquired a ref to old again */

get(old), put(vm), put(old) - okay, you seem to be right indeed.

Fix a typo in subject and:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Patch
diff mbox series

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);
 	}
 
 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);