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

[Oscar Mateo <oscar.mateo@intel.com>]

On 01/11/2018 01:10 PM, Daniele Ceraolo Spurio wrote:
> 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).
>

I was actually thinking of a different fate for this patch: leave patch 
15 as is (maybe make the TODO more open, like "TODO: decide what to do 
with sw_counter"), slap a "drm/i915/icl/guc" prefix on this one and 
consider it together with the rest of the GuC patches. At least in the 
meantime, while  we decide how to go about sw_counter (CCing Tvrtko). 
What do you think?

> 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;
>>     error_ring_free:
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx