Hi Chris,
> All pinning must be done prior to i915_request_create, to avoid
> timeline->mutex inversions.
>
> Here we slightly abuse the context_barrier_task stages to utilise the
> 'skip' decision as an opportunity to acquire the pin on the new ppgtt.
> Consider it s/skip/prepare/. At the moment, we only have on user of
> context_barrier_task, so it might be worth breaking it down for the
> specific task of set-vm and refactor it later if we find a second
> purpose.
[...]
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 9f1dc96b10a6..9d8d75765ee4 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1141,8 +1141,6 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> *cs++ = MI_NOOP;
> intel_ring_advance(rq, cs);
> } else {
> - /* ppGTT is not part of the legacy context image */
> - gen6_ppgtt_pin(i915_vm_to_ppgtt(vm));
> }
mh? Am I not seeing something obvious? Can we remove the else?
>
> return 0;
> @@ -1150,10 +1148,20 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>
> static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> {
> + if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
> + return true;
> +
> if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> - return !ce->state;
> - else
> - return !atomic_read(&ce->pin_count);
> + return false;
> +
> + if (!atomic_read(&ce->pin_count))
> + return true;
> +
> + /* ppGTT is not part of the legacy context image */
> + if (gen6_ppgtt_pin(i915_vm_to_ppgtt(ce->vm)))
> + return true;
> +
> + return false;
looks correct, a bit tricky, but I don't see any issue.
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxx>
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
