[Intel-gfx] [PATCH 03/23] drm/i915: Remove lrc default desc from GEM context

[Tvrtko Ursulin]

On 01/08/2019 17:00, Chris Wilson wrote:
> Quoting Chris Wilson (2019-08-01 16:48:33)
>> Quoting Tvrtko Ursulin (2019-08-01 16:29:53)
>>> For instance Icelake engine dependent stuff sneaked into
>>> intel_lrc.c/lrc_desriptors at some point, which is also against the
>>> spirit of caching. If we were to move the cached value in ce then we
>>> would be able to remove that and have it once again minimal in there.
>>
>> Well we can set all bits but hw_id/lrca at init time. How about if I run
>> that past you?
> 
>   static u64
> -lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
> +base_lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
>   {
> -       struct i915_gem_context *ctx = ce->gem_context;
>          u64 desc;
> 
>          BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (BIT(GEN8_CTX_ID_WIDTH)));
> @@ -426,18 +425,12 @@ lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
>          if (IS_GEN(engine->i915, 8))
>                  desc |= GEN8_CTX_L3LLC_COHERENT;
> 
> -       desc |= i915_ggtt_offset(ce->state) + LRC_HEADER_PAGES * PAGE_SIZE;
> -                                                               /* bits 12-31 */
>          /*
>           * The following 32bits are copied into the OA reports (dword 2).
>           * Consider updating oa_get_render_ctx_id in i915_perf.c when changing
>           * anything below.
>           */
>          if (INTEL_GEN(engine->i915) >= 11) {
> -               GEM_BUG_ON(ctx->hw_id >= BIT(GEN11_SW_CTX_ID_WIDTH));
> -               desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
> -                                                               /* bits 37-47 */
> -
>                  desc |= (u64)engine->instance << GEN11_ENGINE_INSTANCE_SHIFT;
>                                                                  /* bits 48-53 */
> 
> @@ -445,8 +438,29 @@ lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
> 
>                  desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
>                                                                  /* bits 61-63 */
> +       }
> +
> +       return desc;
> +}
> +
> +static u64
> +update_lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
> +{
> +       struct i915_gem_context *ctx = ce->gem_context;
> +       u64 desc = ce->lrc_desc;
> +
> +       desc &= ~GENMASK_ULL(31, 12);
> +       desc |= i915_ggtt_offset(ce->state) + LRC_HEADER_PAGES * PAGE_SIZE;
> +
> +       if (INTEL_GEN(engine->i915) >= 11) {
> +               GEM_BUG_ON(ctx->hw_id >= BIT(GEN11_SW_CTX_ID_WIDTH));
> +
> +               desc &= ~GENMASK_ULL(47, 37);
> +               desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
>          } else {
>                  GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH));
> +
> +               desc &= ~GENMASK_ULL(52, 32);
>                  desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT;   /* bits 32-52 */
>          }
> 
> @@ -1631,7 +1645,7 @@ __execlists_context_pin(struct intel_context *ce,
>          if (ret)
>                  goto unpin_map;
> 
> -       ce->lrc_desc = lrc_descriptor(ce, engine);
> +       ce->lrc_desc = update_lrc_descriptor(ce, engine);
>          ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>          __execlists_update_reg_state(ce, engine);
> 
> @@ -3126,6 +3140,8 @@ static int execlists_context_deferred_alloc(struct intel_context *ce,
>          ce->ring = ring;
>          ce->state = vma;
> 
> +       ce->lrc_desc = base_lrc_descriptor(ce, engine);
> +
>          return 0;
> 
>   error_ring_free:
> 
> That's pretty much the same amount of work in context_pin. I'm not
> convinced that caching between pins achieves very much.
> 
> Concur?

Who kidnapped real Chris? :D We could merge the mask clearing and reduce 
pin to one conditional and one and, shift, or. :)

Okay, have it your way.

Regards,

Tvrtko