[Intel-gfx] [PATCH 16/19] drm/i915/guc: Use a single pass to calculate regset
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Tue Feb 1 22:42:20 UTC 2022
On 1/26/2022 12:36 PM, Lucas De Marchi wrote:
> The ADS initialitazion was using 2 passes to calculate the regset sent
> to GuC to initialize each engine: the first pass to just have the final
> object size and the second to set each register in place in the final
> gem object.
>
> However in order to maintain an ordered set of registers to pass to guc,
> each register needs to be added and moved in the final array. The second
> phase may actually happen in IO memory rather than system memory and
> accessing IO memory by simply dereferencing the pointer doesn't work on
> all architectures. Other places of the ADS initializaition were
> converted to use the dma_buf_map API, but here there may be a lot more
> accesses to IO memory. So, instead of following that same approach,
> convert the regset initialization to calculate the final array in 1
> pass and in the second pass that array is just copied to its final
> location, updating the pointers for each engine written to the ADS blob.
>
> One important thing is that struct temp_regset now have
> different semantics: `registers` continues to track the registers of a
> single engine, however the other fields are updated together, according
> to the newly added `storage`, which tracks the memory allocated for
> all the registers. So rename some of these fields and add a
> __mmio_reg_add(): this function (possibly) allocates memory and operates
> on the storage pointer while guc_mmio_reg_add() continues to manage the
> registers pointer.
>
> On a Tiger Lake system using enable_guc=3, the following log message is
> now seen:
>
> [ 187.334310] i915 0000:00:02.0: [drm:intel_guc_ads_create [i915]] Used 4 KB for temporary ADS regset
>
> This change has also been tested on an ARM64 host with DG2 and other
> discrete graphics cards.
>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 7 ++
> drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 117 +++++++++++++--------
> 2 files changed, 79 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index e2e0df1c3d91..4c852eee3ad8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -152,6 +152,13 @@ struct intel_guc {
> struct dma_buf_map ads_map;
> /** @ads_regset_size: size of the save/restore regsets in the ADS */
> u32 ads_regset_size;
> + /**
> + * @ads_regset_count: number of save/restore registers in the ADS for
> + * each engine
> + */
> + u32 ads_regset_count[I915_NUM_ENGINES];
> + /** @ads_regset: save/restore regsets in the ADS */
> + struct guc_mmio_reg *ads_regset;
> /** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
> u32 ads_golden_ctxt_size;
> /** @ads_engine_usage_size: size of engine usage in the ADS */
> 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 73ca34de44f7..390101ee3661 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -226,14 +226,13 @@ static void guc_mapping_table_init(struct intel_gt *gt,
>
> /*
> * The save/restore register list must be pre-calculated to a temporary
> - * buffer of driver defined size before it can be generated in place
> - * inside the ADS.
> + * buffer before it can be copied inside the ADS.
> */
> -#define MAX_MMIO_REGS 128 /* Arbitrary size, increase as needed */
> struct temp_regset {
> struct guc_mmio_reg *registers;
> - u32 used;
> - u32 size;
> + struct guc_mmio_reg *storage;
I think this could use a comment to distinguish between registers and
storage. Something like.:
/* ptr to the base of the allocated storage for all engines */
struct guc_mmio_reg *storage;
/* ptr to the section of the storage for the engine currently being
worked on */
struct guc_mmio_reg *registers;
> + u32 storage_used;
> + u32 storage_max;
> };
>
> static int guc_mmio_reg_cmp(const void *a, const void *b)
> @@ -244,18 +243,44 @@ static int guc_mmio_reg_cmp(const void *a, const void *b)
> return (int)ra->offset - (int)rb->offset;
> }
>
> +static struct guc_mmio_reg * __must_check
> +__mmio_reg_add(struct temp_regset *regset, struct guc_mmio_reg *reg)
> +{
> + u32 pos = regset->storage_used;
> + struct guc_mmio_reg *slot;
> +
> + if (pos >= regset->storage_max) {
> + size_t size = ALIGN((pos + 1) * sizeof(*slot), PAGE_SIZE);
> + struct guc_mmio_reg *r = krealloc(regset->storage,
> + size, GFP_KERNEL);
> + if (!r) {
> + WARN_ONCE(1, "Incomplete regset list: can't add register (%d)\n",
> + -ENOMEM);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + regset->registers = r + (regset->registers - regset->storage);
> + regset->storage = r;
> + regset->storage_max = size / sizeof(*slot);
> + }
> +
> + slot = ®set->storage[pos];
> + regset->storage_used++;
> + *slot = *reg;
> +
> + return slot;
> +}
> +
> static long __must_check guc_mmio_reg_add(struct temp_regset *regset,
> u32 offset, u32 flags)
> {
> - u32 count = regset->used;
> + u32 count = regset->storage_used - (regset->registers - regset->storage);
> struct guc_mmio_reg reg = {
> .offset = offset,
> .flags = flags,
> };
> struct guc_mmio_reg *slot;
>
> - GEM_BUG_ON(count >= regset->size);
> -
> /*
> * 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
> @@ -266,9 +291,9 @@ static long __must_check guc_mmio_reg_add(struct temp_regset *regset,
> sizeof(reg), guc_mmio_reg_cmp))
> return 0;
>
> - slot = ®set->registers[count];
> - regset->used++;
> - *slot = reg;
> + slot = __mmio_reg_add(regset, ®);
> + if (IS_ERR(slot))
> + return PTR_ERR(slot);
>
> while (slot-- > regset->registers) {
> GEM_BUG_ON(slot[0].offset == slot[1].offset);
> @@ -295,7 +320,11 @@ static int guc_mmio_regset_init(struct temp_regset *regset,
> unsigned int i;
> int ret = 0;
>
> - regset->used = 0;
> + /*
> + * Each engine's registers point to a new start relative to
> + * storage
> + */
> + regset->registers = regset->storage + regset->storage_used;
>
> ret |= GUC_MMIO_REG_ADD(regset, RING_MODE_GEN7(base), true);
> ret |= GUC_MMIO_REG_ADD(regset, RING_HWS_PGA(base), false);
> @@ -317,32 +346,28 @@ static int guc_mmio_regset_init(struct temp_regset *regset,
> return ret ? -1 : 0;
> }
>
> -static int guc_mmio_reg_state_query(struct intel_guc *guc)
> +static long guc_mmio_reg_state_create(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> - struct temp_regset temp_set;
> - u32 total;
> + struct temp_regset temp_set = {};
> + long total = 0;
>
> - /*
> - * Need to actually build the list in order to filter out
> - * duplicates and other such data dependent constructions.
> - */
> - temp_set.size = MAX_MMIO_REGS;
> - temp_set.registers = kmalloc_array(temp_set.size,
> - sizeof(*temp_set.registers),
> - GFP_KERNEL);
> - if (!temp_set.registers)
> - return -ENOMEM;
> -
> - total = 0;
> for_each_engine(engine, gt, id) {
> - guc_mmio_regset_init(&temp_set, engine);
> - total += temp_set.used;
> + u32 used = temp_set.storage_used;
> +
> + if (guc_mmio_regset_init(&temp_set, engine) < 0)
> + return -1;
If you fail here you're leaking temp_set.storage.
Also, any reason not to just return the return code from
guc_mmio_regset_init?
Apart from these minor comments, the change LGTM. IMO we could also
merge this patch on its own ahead of the rest of the dma_buf code,
because not having to recreate the regset on every reset/resume is still
helpful.
Daniele
> +
> + guc->ads_regset_count[id] = temp_set.storage_used - used;
> + total += guc->ads_regset_count[id];
> }
>
> - kfree(temp_set.registers);
> + guc->ads_regset = temp_set.storage;
> +
> + drm_dbg(&guc_to_gt(guc)->i915->drm, "Used %lu KB for temporary ADS regset\n",
> + (temp_set.storage_max * sizeof(struct guc_mmio_reg)) >> 10);
>
> return total * sizeof(struct guc_mmio_reg);
> }
> @@ -352,40 +377,38 @@ static void guc_mmio_reg_state_init(struct intel_guc *guc,
> {
> struct intel_gt *gt = guc_to_gt(guc);
> struct intel_engine_cs *engine;
> + struct guc_mmio_reg *ads_registers;
> enum intel_engine_id id;
> - struct temp_regset temp_set;
> - struct guc_mmio_reg_set *ads_reg_set;
> u32 addr_ggtt, offset;
> - u8 guc_class;
>
> offset = guc_ads_regset_offset(guc);
> addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> - temp_set.registers = (struct guc_mmio_reg *)(((u8 *)blob) + offset);
> - temp_set.size = guc->ads_regset_size / sizeof(temp_set.registers[0]);
> + ads_registers = (struct guc_mmio_reg *)(((u8 *)blob) + offset);
> +
> + memcpy(ads_registers, guc->ads_regset, guc->ads_regset_size);
>
> for_each_engine(engine, gt, id) {
> + u32 count = guc->ads_regset_count[id];
> + struct guc_mmio_reg_set *ads_reg_set;
> + u8 guc_class;
> +
> /* Class index is checked in class converter */
> GEM_BUG_ON(engine->instance >= GUC_MAX_INSTANCES_PER_CLASS);
>
> guc_class = engine_class_to_guc_class(engine->class);
> ads_reg_set = &blob->ads.reg_state_list[guc_class][engine->instance];
>
> - guc_mmio_regset_init(&temp_set, engine);
> - if (!temp_set.used) {
> + if (!count) {
> ads_reg_set->address = 0;
> ads_reg_set->count = 0;
> continue;
> }
>
> ads_reg_set->address = addr_ggtt;
> - ads_reg_set->count = temp_set.used;
> + ads_reg_set->count = count;
>
> - temp_set.size -= temp_set.used;
> - temp_set.registers += temp_set.used;
> - addr_ggtt += temp_set.used * sizeof(struct guc_mmio_reg);
> + addr_ggtt += count * sizeof(struct guc_mmio_reg);
> }
> -
> - GEM_BUG_ON(temp_set.size);
> }
>
> static void fill_engine_enable_masks(struct intel_gt *gt,
> @@ -634,8 +657,11 @@ int intel_guc_ads_create(struct intel_guc *guc)
>
> GEM_BUG_ON(guc->ads_vma);
>
> - /* Need to calculate the reg state size dynamically: */
> - ret = guc_mmio_reg_state_query(guc);
> + /*
> + * Create reg state size dynamically on system memory to be copied to
> + * the final ads blob on gt init/reset
> + */
> + ret = guc_mmio_reg_state_create(guc);
> if (ret < 0)
> return ret;
> guc->ads_regset_size = ret;
> @@ -681,6 +707,7 @@ void intel_guc_ads_destroy(struct intel_guc *guc)
> i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP);
> guc->ads_blob = NULL;
> dma_buf_map_clear(&guc->ads_map);
> + kfree(guc->ads_regset);
> }
>
> static void guc_ads_private_data_reset(struct intel_guc *guc)
More information about the Intel-gfx
mailing list