Re: [Intel-gfx] [PATCH] drm/i915: Make lrc_init_wa_ctx compatible with ww locking, v3.

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Maarten Lankhorst (2021-02-01 12:50:37)
> Make creation separate from pinning, in order to take the lock only
> once, and pin the mapping with the lock held.
> 
> Changes since v1:
> - Rebase on top of upstream changes.
> Changes since v2:
> - Fully clear wa_ctx on error.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 49 ++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 8508b8d701c1..a2b916d27a39 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1421,7 +1421,7 @@ gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>  
>  #define CTX_WA_BB_SIZE (PAGE_SIZE)
>  
> -static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
> +static int lrc_create_wa_ctx(struct intel_engine_cs *engine)
>  {
>         struct drm_i915_gem_object *obj;
>         struct i915_vma *vma;
> @@ -1437,10 +1437,6 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
>                 goto err;
>         }
>  
> -       err = i915_ggtt_pin(vma, NULL, 0, PIN_HIGH);
> -       if (err)
> -               goto err;
> -
>         engine->wa_ctx.vma = vma;
>         return 0;
>  
> @@ -1452,9 +1448,6 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
>  void lrc_fini_wa_ctx(struct intel_engine_cs *engine)
>  {
>         i915_vma_unpin_and_release(&engine->wa_ctx.vma, 0);
> -
> -       /* Called on error unwind, clear all flags to prevent further use */
> -       memset(&engine->wa_ctx, 0, sizeof(engine->wa_ctx));
>  }
>  
>  typedef u32 *(*wa_bb_func_t)(struct intel_engine_cs *engine, u32 *batch);
> @@ -1466,6 +1459,7 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine)
>                 &wa_ctx->indirect_ctx, &wa_ctx->per_ctx
>         };
>         wa_bb_func_t wa_bb_fn[ARRAY_SIZE(wa_bb)];
> +       struct i915_gem_ww_ctx ww;
>         void *batch, *batch_ptr;
>         unsigned int i;
>         int err;
> @@ -1494,7 +1488,7 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine)
>                 return;
>         }
>  
> -       err = lrc_setup_wa_ctx(engine);
> +       err = lrc_create_wa_ctx(engine);
>         if (err) {
>                 /*
>                  * We continue even if we fail to initialize WA batch
> @@ -1507,7 +1501,22 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine)
>                 return;
>         }
>  
> +       if (!engine->wa_ctx.vma)
> +               return;
> +
> +       i915_gem_ww_ctx_init(&ww, true);
> +retry:
> +       err = i915_gem_object_lock(wa_ctx->vma->obj, &ww);
> +       if (!err)
> +               err = i915_ggtt_pin(wa_ctx->vma, &ww, 0, PIN_HIGH);
> +       if (err)
> +               goto err;
> +
>         batch = i915_gem_object_pin_map(wa_ctx->vma->obj, I915_MAP_WB);

Given that the pages are already pinned and must remain pinned for the
lifetime of the engine, and the ww is not used here to setup the CPU
page tables, fix the lack of primitives.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx