Re: [PATCH 20/27] drm/i915/icl: Make use of the SW counter field in the new context descriptor

Daniele Ceraolo Spurio
2018-01-11T21:10:25Z

This could potentially be squashed with patch 15, as it doesn't make
much sense to add a TODO there and solve it here. We might also want to
update the comment above intel_lr_context_descriptor_update to remove
the implication that SW context ID == ctx->hw_id (which is still
technically true after this patch but we're preparing for it not to be
anymore).

Thanks,
Daniele

On 09/01/18 15:28, Paulo Zanoni wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
>
> The new context descriptor format in Gen11 contains two assignable fields: the
> SW Context ID (technically 11 bits, but practically limited to 2032 entries due
> to some being reserved for future use by the GuC) and the SW Counter (6 bits).
>
> We don't want to limit ourselves too much in the maximum number of concurrent
> contexts we want to allow, so ideally we want to employ every possible bit
> available. Unfortunately, a further limitation in the interface with the GuC
> means the combination of SW Context ID + SW Counter has to be unique within the
> same engine class (as we use the SW Context ID to index in the GuC stage
> descriptor pool, and the Engine Class + SW Counter to index in the 2-dimensional
> lrc array). This essentially means we need to somehow encode the engine instance.
>
> Since the BSpec allows 6 bits for engine instance, we use the whole SW counter
> for this task. If the limitation of 2032 maximum simultaneous contexts is too
> restrictive, we can always squeeze things a bit more (3 extras bits for hw_id,
> 3 bits for instance) and things will still work (Gen11 does not instance more
> than 8 engines of any class).
>
> Another alternative would be to generate the hw_id per HW context instead of per
> GEM context, but that has other problems (e.g. maximum number of user-created
> contexts would be variable, no relationship between a GuC principal descriptor
> and the proxy descriptor it uses, etc...).
>
> Bspec: 12254
>
> v2:
> - Squashed with parts of "Interface changes for GuC fw 22.108" (Daniele)
> - Do not apply the 16 reserved entries limitation to the non-GuC path (Joonas)
> v3: Rebased by Rodrigo.
> v4: Rebased (s/i915_modparams.enable_guc_submission/USES_GUC_SUBMISSION(dev_priv))
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++---
> drivers/gpu/drm/i915/i915_gem_context.c | 9 ++++++---
> drivers/gpu/drm/i915/i915_gem_context.h | 2 ++
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_lrc.c | 15 ++++++++-------
> 5 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index aa4f2b178d97..3f1d8c0d2b0a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2079,11 +2079,16 @@ struct drm_i915_private {
>
> /* The hw wants to have a stable context identifier for the
> * lifetime of the context (for OA, PASID, faults, etc).
> - * This is limited in execlists to 21 bits.
> + * This is limited in execlists to 21 bits. In enhanced execlist
> + * (GEN11+) this is limited to 11 bits (the SW Context ID field)
> + * but GuC limits it a bit further (11 bits - 16) due to some
> + * entries being reserved for future use (so the firmware only
> + * supports a GuC stage descriptor pool of 2032 entries).
> */
> struct ida hw_ida;
> -#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> -#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> +#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> +#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> +#define GEN11_MAX_CONTEXT_HW_ID_WITH_GUC GEN11_MAX_CONTEXT_HW_ID - 16
> } contexts;
>
> u32 fdi_rx_config;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index dbc50b9e18c9..bb5d070083f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -213,9 +213,12 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
> int ret;
> unsigned int max;
>
> - if (INTEL_GEN(dev_priv) >= 11)
> - max = GEN11_MAX_CONTEXT_HW_ID;
> - else
> + if (INTEL_GEN(dev_priv) >= 11) {
> + if (USES_GUC_SUBMISSION(dev_priv))
> + max = GEN11_MAX_CONTEXT_HW_ID_WITH_GUC;
> + else
> + max = GEN11_MAX_CONTEXT_HW_ID;
> + } else
> max = MAX_CONTEXT_HW_ID;
>
> ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 4bfb72f8e1cb..7a39d54e9962 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -156,6 +156,8 @@ struct i915_gem_context {
> struct intel_ring *ring;
> u32 *lrc_reg_state;
> u64 lrc_desc;
> + u32 sw_context_id;
> + u32 sw_counter;
> int pin_count;
> } engine[I915_NUM_ENGINES];
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d8b537570b8e..6d5e2c651580 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3860,6 +3860,8 @@ enum {
> #define GEN8_CTX_ID_WIDTH 21
> #define GEN11_SW_CTX_ID_SHIFT 37
> #define GEN11_SW_CTX_ID_WIDTH 11
> +#define GEN11_SW_COUNTER_SHIFT 55
> +#define GEN11_SW_COUNTER_WIDTH 6
> #define GEN11_ENGINE_CLASS_SHIFT 61
> #define GEN11_ENGINE_INSTANCE_SHIFT 48
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d527a79c872c..edf050de8ffe 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -267,15 +267,11 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> if (INTEL_GEN(ctx->i915) >= 11) {
> desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
> /* bits 61-63 */
> -
> - /*
> - * TODO: use SW counter (bits 60-55) to support more CTXs by
> - * combining it with the SW context ID field?
> - */
> -
> + desc |= (u64)ce->sw_counter << GEN11_SW_COUNTER_SHIFT;
> + /* bits 55-60 */
> desc |= (u64)engine->instance << GEN11_ENGINE_INSTANCE_SHIFT;
> /* bits 53-48 */
> - desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
> + desc |= (u64)ce->sw_context_id << GEN11_SW_CTX_ID_SHIFT;
> /* bits 37-47 */
> } else {
> desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-52 */
> @@ -2398,6 +2394,11 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> ce->ring = ring;
> ce->state = vma;
>
> + if (INTEL_GEN(ctx->i915) >= 11) {
> + ce->sw_context_id = ctx->hw_id;
> + ce->sw_counter = engine->instance;
> + }
> +
> return 0;
>