Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> 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.
>
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@xxxxxxxxxxxxxxx>
> 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
