Quoting Mika Kuoppala (2019-06-14 10:22:16)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
