[PATCH 3/3] drm/i915: Remove intel_context->ops->(pre_pin/post_unpin)
Matthew Brost
matthew.brost at intel.com
Wed Jun 30 01:25:55 UTC 2021
On Thu, Jun 17, 2021 at 09:13:19AM +0200, Maarten Lankhorst wrote:
> Now that intel_context->pin_mutex is gone, the reason for splitting
> pre_pin/post_unpin ops is also gone. Remove those ops, and handle
> this detail inside guc/execlists submission only.
>
A few questions comments about this series / pinning code in general.
Do we still need intel_context_pre_pin? Can't we do everything in
i915_active_acquire(ce->active) now (__intel_context_active)? I ask
because I have concern we may not be able hide whole multi-lrc pinning
differences entirely in the vfunc ce->pin like we should. Not 100% on
this though without coding it up.
For multi-lrc we will likely need a vfunc to do the ww pinning as with
multi-lrc if any of the contexts need to be pinned, all the contexts in
the relationship need to be pinned all the contexts need to pinned.
Neither of these are blockers. Please respond and I'll likely give an
RB for this patch and previous as this a step in the right direction.
Matt
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_context.c | 19 ++-----
> drivers/gpu/drm/i915/gt/intel_context_types.h | 4 +-
> .../drm/i915/gt/intel_execlists_submission.c | 50 ++++++++-----------
> .../gpu/drm/i915/gt/intel_ring_submission.c | 16 +-----
> drivers/gpu/drm/i915/gt/mock_engine.c | 14 +-----
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 ++++++----
> 6 files changed, 44 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index e6dab37c4266..b630c1968794 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -207,8 +207,6 @@ static void intel_context_post_unpin(struct intel_context *ce)
> int __intel_context_do_pin_ww(struct intel_context *ce,
> struct i915_gem_ww_ctx *ww)
> {
> - bool handoff = false;
> - void *vaddr;
> int err;
>
> err = intel_context_alloc_state(ce);
> @@ -229,40 +227,32 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
> if (err)
> goto err_ctx_unpin;
>
> - err = ce->ops->pre_pin(ce, ww, &vaddr);
> - if (err)
> - goto err_release;
> -
> if (unlikely(intel_context_is_closed(ce))) {
> err = -ENOENT;
> - goto err_post_unpin;
> + goto err_release;
> }
>
> if (likely(!atomic_add_unless(&ce->pin_count, 1, 0))) {
> err = intel_context_active_acquire(ce);
> if (unlikely(err))
> - goto err_post_unpin;
> + goto err_release;
>
> - err = ce->ops->pin(ce, vaddr);
> + err = ce->ops->pin(ce, ww);
> if (err) {
> intel_context_active_release(ce);
> - goto err_post_unpin;
> + goto err_release;
> }
>
> CE_TRACE(ce, "pin ring:{start:%08x, head:%04x, tail:%04x}\n",
> i915_ggtt_offset(ce->ring->vma),
> ce->ring->head, ce->ring->tail);
>
> - handoff = true;
> smp_mb__before_atomic(); /* flush pin before it is visible */
> atomic_inc(&ce->pin_count);
> }
>
> GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
>
> -err_post_unpin:
> - if (!handoff)
> - ce->ops->post_unpin(ce);
> err_release:
> i915_active_release(&ce->active);
> err_ctx_unpin:
> @@ -303,7 +293,6 @@ void intel_context_unpin(struct intel_context *ce)
>
> CE_TRACE(ce, "unpin\n");
> ce->ops->unpin(ce);
> - ce->ops->post_unpin(ce);
>
> /*
> * Once released, we may asynchronously drop the active reference.
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index b104d5e9d3b6..e10057901c6c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -35,10 +35,8 @@ struct intel_context_ops {
>
> int (*alloc)(struct intel_context *ce);
>
> - int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **vaddr);
> - int (*pin)(struct intel_context *ce, void *vaddr);
> + int (*pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww);
> void (*unpin)(struct intel_context *ce);
> - void (*post_unpin)(struct intel_context *ce);
>
> void (*enter)(struct intel_context *ce);
> void (*exit)(struct intel_context *ce);
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index fc77592d88a9..7c73a0ff4729 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -2497,35 +2497,38 @@ static void execlists_submit_request(struct i915_request *request)
> }
>
> static int
> -__execlists_context_pre_pin(struct intel_context *ce,
> - struct intel_engine_cs *engine,
> - struct i915_gem_ww_ctx *ww, void **vaddr)
> +__execlists_context_pin(struct intel_context *ce,
> + struct intel_engine_cs *engine,
> + struct i915_gem_ww_ctx *ww)
> {
> int err;
> + void *vaddr;
>
> - err = lrc_pre_pin(ce, engine, ww, vaddr);
> + err = lrc_pre_pin(ce, engine, ww, &vaddr);
> if (err)
> return err;
>
> if (!__test_and_set_bit(CONTEXT_INIT_BIT, &ce->flags)) {
> - lrc_init_state(ce, engine, *vaddr);
> + lrc_init_state(ce, engine, vaddr);
>
> __i915_gem_object_flush_map(ce->state->obj, 0, engine->context_size);
> }
>
> - return 0;
> + err = lrc_pin(ce, ce->engine, vaddr);
> + if (err)
> + lrc_post_unpin(ce);
> + return err;
> }
>
> -static int execlists_context_pre_pin(struct intel_context *ce,
> - struct i915_gem_ww_ctx *ww,
> - void **vaddr)
> +static int execlists_context_pin(struct intel_context *ce, struct i915_gem_ww_ctx *ww)
> {
> - return __execlists_context_pre_pin(ce, ce->engine, ww, vaddr);
> + return __execlists_context_pin(ce, ce->engine, ww);
> }
>
> -static int execlists_context_pin(struct intel_context *ce, void *vaddr)
> +static void execlists_context_unpin(struct intel_context *ce)
> {
> - return lrc_pin(ce, ce->engine, vaddr);
> + lrc_unpin(ce);
> + lrc_post_unpin(ce);
> }
>
> static int execlists_context_alloc(struct intel_context *ce)
> @@ -2533,15 +2536,14 @@ static int execlists_context_alloc(struct intel_context *ce)
> return lrc_alloc(ce, ce->engine);
> }
>
> +
> static const struct intel_context_ops execlists_context_ops = {
> .flags = COPS_HAS_INFLIGHT,
>
> .alloc = execlists_context_alloc,
>
> - .pre_pin = execlists_context_pre_pin,
> .pin = execlists_context_pin,
> - .unpin = lrc_unpin,
> - .post_unpin = lrc_post_unpin,
> + .unpin = execlists_context_unpin,
>
> .enter = intel_context_enter_engine,
> .exit = intel_context_exit_engine,
> @@ -3401,21 +3403,11 @@ static int virtual_context_alloc(struct intel_context *ce)
> return lrc_alloc(ce, ve->siblings[0]);
> }
>
> -static int virtual_context_pre_pin(struct intel_context *ce,
> - struct i915_gem_ww_ctx *ww,
> - void **vaddr)
> -{
> - struct virtual_engine *ve = container_of(ce, typeof(*ve), context);
> -
> - /* Note: we must use a real engine class for setting up reg state */
> - return __execlists_context_pre_pin(ce, ve->siblings[0], ww, vaddr);
> -}
> -
> -static int virtual_context_pin(struct intel_context *ce, void *vaddr)
> +static int virtual_context_pin(struct intel_context *ce, struct i915_gem_ww_ctx *ww)
> {
> struct virtual_engine *ve = container_of(ce, typeof(*ve), context);
>
> - return lrc_pin(ce, ve->siblings[0], vaddr);
> + return __execlists_context_pin(ce, ve->siblings[0], ww);
> }
>
> static void virtual_context_enter(struct intel_context *ce)
> @@ -3445,10 +3437,8 @@ static const struct intel_context_ops virtual_context_ops = {
>
> .alloc = virtual_context_alloc,
>
> - .pre_pin = virtual_context_pre_pin,
> .pin = virtual_context_pin,
> - .unpin = lrc_unpin,
> - .post_unpin = lrc_post_unpin,
> + .unpin = execlists_context_unpin,
>
> .enter = virtual_context_enter,
> .exit = virtual_context_exit,
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 0c423f096e2b..0da3842729ae 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -467,9 +467,8 @@ static int ring_context_init_default_state(struct intel_context *ce,
> return 0;
> }
>
> -static int ring_context_pre_pin(struct intel_context *ce,
> - struct i915_gem_ww_ctx *ww,
> - void **unused)
> +static int ring_context_pin(struct intel_context *ce,
> + struct i915_gem_ww_ctx *ww)
> {
> struct i915_address_space *vm;
> int err = 0;
> @@ -498,10 +497,6 @@ static void __context_unpin_ppgtt(struct intel_context *ce)
> }
>
> static void ring_context_unpin(struct intel_context *ce)
> -{
> -}
> -
> -static void ring_context_post_unpin(struct intel_context *ce)
> {
> __context_unpin_ppgtt(ce);
> }
> @@ -572,11 +567,6 @@ static int ring_context_alloc(struct intel_context *ce)
> return 0;
> }
>
> -static int ring_context_pin(struct intel_context *ce, void *unused)
> -{
> - return 0;
> -}
> -
> static void ring_context_reset(struct intel_context *ce)
> {
> intel_ring_reset(ce->ring, ce->ring->emit);
> @@ -586,10 +576,8 @@ static void ring_context_reset(struct intel_context *ce)
> static const struct intel_context_ops ring_context_ops = {
> .alloc = ring_context_alloc,
>
> - .pre_pin = ring_context_pre_pin,
> .pin = ring_context_pin,
> .unpin = ring_context_unpin,
> - .post_unpin = ring_context_post_unpin,
>
> .enter = intel_context_enter_engine,
> .exit = intel_context_exit_engine,
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index 32589c6625e1..aa0da27c9b3e 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -123,10 +123,6 @@ static void mock_context_unpin(struct intel_context *ce)
> {
> }
>
> -static void mock_context_post_unpin(struct intel_context *ce)
> -{
> -}
> -
> static void mock_context_destroy(struct kref *ref)
> {
> struct intel_context *ce = container_of(ref, typeof(*ce), ref);
> @@ -166,13 +162,7 @@ static int mock_context_alloc(struct intel_context *ce)
> return 0;
> }
>
> -static int mock_context_pre_pin(struct intel_context *ce,
> - struct i915_gem_ww_ctx *ww, void **unused)
> -{
> - return 0;
> -}
> -
> -static int mock_context_pin(struct intel_context *ce, void *unused)
> +static int mock_context_pin(struct intel_context *ce, struct i915_gem_ww_ctx *ww)
> {
> return 0;
> }
> @@ -184,10 +174,8 @@ static void mock_context_reset(struct intel_context *ce)
> static const struct intel_context_ops mock_context_ops = {
> .alloc = mock_context_alloc,
>
> - .pre_pin = mock_context_pre_pin,
> .pin = mock_context_pin,
> .unpin = mock_context_unpin,
> - .post_unpin = mock_context_post_unpin,
>
> .enter = intel_context_enter_engine,
> .exit = intel_context_exit_engine,
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 7c8ff9792f7b..e0e471b22c06 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -437,25 +437,32 @@ static int guc_context_alloc(struct intel_context *ce)
> return lrc_alloc(ce, ce->engine);
> }
>
> -static int guc_context_pre_pin(struct intel_context *ce,
> - struct i915_gem_ww_ctx *ww,
> - void **vaddr)
> +static int guc_context_pin(struct intel_context *ce, struct i915_gem_ww_ctx *ww)
> {
> - return lrc_pre_pin(ce, ce->engine, ww, vaddr);
> + void *vaddr;
> + int err;
> +
> + err = lrc_pre_pin(ce, ce->engine, ww, &vaddr);
> + if (err)
> + return err;
> +
> + err = lrc_pin(ce, ce->engine, vaddr);
> + if (err)
> + lrc_post_unpin(ce);
> + return err;
> }
>
> -static int guc_context_pin(struct intel_context *ce, void *vaddr)
> +static void guc_context_unpin(struct intel_context *ce)
> {
> - return lrc_pin(ce, ce->engine, vaddr);
> + lrc_unpin(ce);
> + lrc_post_unpin(ce);
> }
>
> static const struct intel_context_ops guc_context_ops = {
> .alloc = guc_context_alloc,
>
> - .pre_pin = guc_context_pre_pin,
> .pin = guc_context_pin,
> - .unpin = lrc_unpin,
> - .post_unpin = lrc_post_unpin,
> + .unpin = guc_context_unpin,
>
> .enter = intel_context_enter_engine,
> .exit = intel_context_exit_engine,
> --
> 2.31.0
>
> _______________________________________________
> Intel-gfx-trybot mailing list
> Intel-gfx-trybot at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx-trybot
More information about the Intel-gfx-trybot
mailing list