[Intel-gfx] [PATCH 1/2] drm/i915/guc: Provide mmio list to be saved/restored on engine reset
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Dec 12 23:01:51 UTC 2019
On 12/12/2019 18:43, Fernando Pacheco wrote:
> The driver must provide GuC with a list of mmio registers
> that should be saved/restored during a GuC-based engine reset.
> We provide a minimal set of registers that should get things
> working and extend as needed.
>
> v2: rebase and comment to explain why mmio list is kept sorted
>
> Signed-off-by: Fernando Pacheco <fernando.pacheco at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 25 +++--
> .../gpu/drm/i915/gt/intel_workarounds_types.h | 1 +
> drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 96 ++++++++++++++++++-
> 3 files changed, 114 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 195ccf7db272..866d4a7ba0ea 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -148,29 +148,37 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
> }
>
> static void wa_add(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> - u32 val, u32 read_mask)
> + u32 val, u32 read_mask, bool masked_bits)
> {
> struct i915_wa wa = {
> .reg = reg,
> .mask = mask,
> .val = val,
> .read = read_mask,
> + .masked_bits = masked_bits,
> };
>
> _wa_add(wal, &wa);
> }
>
> +static void
> +__wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> + u32 val, bool masked_bits)
> +{
> + wa_add(wal, reg, mask, val, mask, masked_bits);
> +}
> +
> static void
> wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> u32 val)
> {
> - wa_add(wal, reg, mask, val, mask);
> + __wa_write_masked_or(wal, reg, mask, val, false);
> }
>
> static void
> wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> {
> - wa_write_masked_or(wal, reg, val, _MASKED_BIT_ENABLE(val));
> + __wa_write_masked_or(wal, reg, val, _MASKED_BIT_ENABLE(val), true);
> }
>
> static void
> @@ -186,13 +194,16 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> }
>
> #define WA_SET_BIT_MASKED(addr, mask) \
> - wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
> + __wa_write_masked_or(wal, (addr), (mask), \
> + _MASKED_BIT_ENABLE(mask), true)
>
> #define WA_CLR_BIT_MASKED(addr, mask) \
> - wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_DISABLE(mask))
> + __wa_write_masked_or(wal, (addr), (mask), \
> + _MASKED_BIT_DISABLE(mask), true)
>
> #define WA_SET_FIELD_MASKED(addr, mask, value) \
> - wa_write_masked_or(wal, (addr), (mask), _MASKED_FIELD((mask), (value)))
> + __wa_write_masked_or(wal, (addr), (mask), \
> + _MASKED_FIELD((mask), (value)), true)
>
> static void gen8_ctx_workarounds_init(struct intel_engine_cs *engine,
> struct i915_wa_list *wal)
> @@ -592,7 +603,7 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> */
> wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
> - FF_MODE2_TDS_TIMER_MASK);
> + FF_MODE2_TDS_TIMER_MASK, false);
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
> index e27ab1b710b3..a43d5f968f2d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
> @@ -16,6 +16,7 @@ struct i915_wa {
> u32 mask;
> u32 val;
> u32 read;
> + bool masked_bits;
> };
>
> struct i915_wa_list {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 101728006ae9..3fea13fc2b1a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -3,6 +3,8 @@
> * Copyright © 2014-2019 Intel Corporation
> */
>
> +#include <linux/bsearch.h>
> +
> #include "gt/intel_gt.h"
> #include "intel_guc_ads.h"
> #include "intel_uc.h"
> @@ -16,6 +18,9 @@
> * its internal state for sleep.
> */
>
> +static void guc_mmio_reg_state_init(struct guc_mmio_reg_state *reg_state,
> + struct intel_engine_cs *engine);
> +
> static void guc_policy_init(struct guc_policy *policy)
> {
> policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> @@ -67,12 +72,19 @@ struct __guc_ads_blob {
>
> static void __guc_ads_init(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_gt(guc)->i915;
> + struct intel_gt *gt = guc_to_gt(guc);
> + struct drm_i915_private *dev_priv = gt->i915;
> struct __guc_ads_blob *blob = guc->ads_blob;
> const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> u32 base;
> u8 engine_class;
>
> + /* GuC mmio save/restore list */
> + for_each_engine(engine, gt, id)
> + guc_mmio_reg_state_init(&blob->reg_state, engine);
> +
> /* GuC scheduling policies */
> guc_policies_init(&blob->policies);
>
> @@ -170,3 +182,85 @@ void intel_guc_ads_reset(struct intel_guc *guc)
> return;
> __guc_ads_init(guc);
> }
> +
> +static int guc_mmio_reg_cmp(const void *a, const void *b)
> +{
> + const struct guc_mmio_reg *ra = a;
> + const struct guc_mmio_reg *rb = b;
> +
> + return (int)ra->offset - (int)rb->offset;
> +}
> +
> +static void guc_mmio_reg_add(struct guc_mmio_regset *regset,
> + u32 offset, u32 flags)
> +{
> + u32 count = regset->number_of_registers;
> + struct guc_mmio_reg reg = {
> + .offset = offset,
> + .flags = flags,
> + };
> + struct guc_mmio_reg *slot;
> +
> + GEM_BUG_ON(count >= GUC_REGSET_MAX_REGISTERS);
> +
> + /*
> + * The mmio list is built using separate lists within the driver.
> + * It's possible that at some point we may attempt to add the same
> + * register more than once. Do not consider this an error; silently
> + * move on if the register is already in the list.
> + */
> + if (bsearch(®, regset->registers, count,
> + sizeof(reg), guc_mmio_reg_cmp))
> + return;
> +
> + slot = ®set->registers[count];
> + regset->number_of_registers++;
> + *slot = reg;
> +
> + while (slot-- > regset->registers) {
> + GEM_BUG_ON(slot[0].offset == slot[1].offset);
> + if (slot[1].offset > slot[0].offset)
> + break;
> +
> + swap(slot[1], slot[0]);
> + }
> +}
> +
> +#define GUC_MMIO_REG_ADD(regset, reg, masked) \
> + guc_mmio_reg_add(regset, \
> + i915_mmio_reg_offset((reg)), \
> + (masked) ? GUC_REGSET_MASKED : 0)
> +
> +static void guc_mmio_regset_init(struct guc_mmio_regset *regset,
> + struct intel_engine_cs *engine)
> +{
> + const u32 base = engine->mmio_base;
> + struct i915_wa_list *wal = &engine->wa_list;
> + struct i915_wa *wa;
> + unsigned int i;
> +
> + GUC_MMIO_REG_ADD(regset, RING_MODE_GEN7(base), true);
> + GUC_MMIO_REG_ADD(regset, RING_HWS_PGA(base), false);
> + GUC_MMIO_REG_ADD(regset, RING_IMR(base), false);
> +
> + for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> + GUC_MMIO_REG_ADD(regset, wa->reg, wa->masked_bits);
> +
> + /* Be extra paranoid and include all whitelist registers. */
> + for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++)
> + GUC_MMIO_REG_ADD(regset,
> + RING_FORCE_TO_NONPRIV(base, i),
> + false);
> +}
> +
> +static void guc_mmio_reg_state_init(struct guc_mmio_reg_state *reg_state,
> + struct intel_engine_cs *engine)
> +{
> + struct guc_mmio_regset *regset;
> +
> + GEM_BUG_ON(engine->class >= GUC_MAX_ENGINE_CLASSES);
> + GEM_BUG_ON(engine->instance >= GUC_MAX_INSTANCES_PER_CLASS);
> + regset = ®_state->engine_reg[engine->class][engine->instance];
> +
> + guc_mmio_regset_init(regset, engine);
> +}
>
So presumably the relevant GuC FW interface structures will remain the
same from upstream to new interface?
Assuming that, the code looks fine so:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
But it is dead code for now, right? Dead in the sense GuC engine reset
path does not get exercised until more new patches land.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list