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

[Chris Wilson]

Quoting Mika Kuoppala (2019-06-14 10:22:16)
> Chris Wilson <chris at 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.

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