Re: [PATCH v2] drm/i915: Keep contexts pinned until after the next kernel context switch

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-06-14 10:22:16)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>> > index c78ec0b58e77..8e299c631575 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>> > @@ -61,7 +61,6 @@ int __intel_context_do_pin(struct intel_context *ce)
>> >  
>> >               i915_gem_context_get(ce->gem_context); /* for ctx->ppgtt */
>> >  
>> > -             intel_context_get(ce);
>> >               smp_mb__before_atomic(); /* flush pin before it is visible */
>> >       }
>> >  
>> > @@ -89,20 +88,45 @@ void intel_context_unpin(struct intel_context *ce)
>> >               ce->ops->unpin(ce);
>> >  
>> >               i915_gem_context_put(ce->gem_context);
>> > -             intel_context_put(ce);
>> > +             intel_context_active_release(ce);
>> 
>> Not going to insist any change in naming but I was thinking
>> here that we arm the barriers.
>
> Not keen, not for changing just _release as we end up with _acquire/_arm
> and that does not seem symmetrical.
>
> _release_deferred() _release_barrier() perhaps, but no need to
> differentiate yet. _release_barrier() winning so far.
>
>> >       mutex_unlock(&ce->pin_mutex);
>> >       intel_context_put(ce);
>> >  }
>> >  
>> > -static void intel_context_retire(struct i915_active_request *active,
>> > -                              struct i915_request *rq)
>> > +static int __context_pin_state(struct i915_vma *vma, unsigned long flags)
>> >  {
>> > -     struct intel_context *ce =
>> > -             container_of(active, typeof(*ce), active_tracker);
>> > +     int err;
>> 
>> Why not ret? I have started to removing errs. Am I swimming in upstream? :P
>
> We've been replacing ret with err (where it makes more sense to ask "if
> (error) do error_path;) for a few years. :-p
>
>> > -     intel_context_unpin(ce);
>> > +     err = i915_vma_pin(vma, 0, 0, flags | PIN_GLOBAL);
>> > +     if (err)
>> > +             return err;
>> > +
>> > +     /*
>> > +      * And mark it as a globally pinned object to let the shrinker know
>> > +      * it cannot reclaim the object until we release it.
>> > +      */
>> > +     vma->obj->pin_global++;
>> > +     vma->obj->mm.dirty = true;
>> > +
>> > +     return 0;
>> > +}
>
>> > +int intel_context_active_acquire(struct intel_context *ce, unsigned long flags)
>> > +{
>> > +     int err;
>> > +
>> > +     if (!i915_active_acquire(&ce->active))
>> > +             return 0;
>> > +
>> > +     intel_context_get(ce);
>> > +
>> > +     if (!ce->state)
>> > +             return 0;
>> > +
>> > +     err = __context_pin_state(ce->state, flags);
>> > +     if (err) {
>> > +             i915_active_cancel(&ce->active);
>> > +             intel_context_put(ce);
>> > +             return err;
>> > +     }
>> > +
>> > +     /* Preallocate tracking nodes */
>> > +     if (!i915_gem_context_is_kernel(ce->gem_context)) {
>> > +             err = i915_active_acquire_preallocate_barrier(&ce->active,
>> > +                                                           ce->engine);
>> > +             if (err) {
>> > +                     i915_active_release(&ce->active);
>> 
>> For me it looks like we are missing context put in here.
>
> Crazy huh :) We are at the point where it is safer to release than
> unwind; i915_active_cancel is quite ugly.
>

Ok so the retirement of active releases the context ref we have.

And you add to the ref->count on moving to the barriers list so
partially done engine masks should still be covered.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> It does get a bit simpler later on when we rewrite i915_active and
> drive this as an acquire callback.
>
>> > diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
>> > index b679253b53a5..c025991b9233 100644
>> > --- a/drivers/gpu/drm/i915/i915_active_types.h
>> > +++ b/drivers/gpu/drm/i915/i915_active_types.h
>> > @@ -7,6 +7,7 @@
>> >  #ifndef _I915_ACTIVE_TYPES_H_
>> >  #define _I915_ACTIVE_TYPES_H_
>> >  
>> > +#include <linux/llist.h>
>> >  #include <linux/rbtree.h>
>> >  #include <linux/rcupdate.h>
>> >  
>> > @@ -31,6 +32,8 @@ struct i915_active {
>> >       unsigned int count;
>> >  
>> >       void (*retire)(struct i915_active *ref);
>> > +
>> > +     struct llist_head barriers;
>> 
>> This looks like it is generic. Are you planning to extend?
>
> Only user so far far. But i915_active is our answer to
> reservation_object on steroids, so itself should be quite generic.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx