Re: [PATCH] drm/i915/guc: Use iosys_map interface to update lrc_desc

[John Harrison <john.c.harrison@xxxxxxxxx>]

Sorry, only just seen this patch.

Please do not do this!

The entire lrc_desc_pool entity is being dropped as part of the update 
to GuC v70. That's why there was a recent patch set to significantly 
re-organise how/where it is used. That patch set explicitly said - this 
is all in preparation for removing the desc pool entirely.

Merging this change would just cause unnecessary churn and rebase 
conflicts with the v70 update patches that I am working on. Please wait 
until that lands and then see if there is anything left that you think 
still needs to be updated.

John.


On 3/8/2022 08:47, Balasubramani Vivekanandan wrote:
> This patch is continuation of the effort to move all pointers in i915,
> which at any point may be pointing to device memory or system memory, to
> iosys_map interface.
> More details about the need of this change is explained in the patch
> series which initiated this task
> https://patchwork.freedesktop.org/series/99711/
>
> This patch converts all access to the lrc_desc through iosys_map
> interfaces.
>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  2 +-
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 68 ++++++++++++-------
>   2 files changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index e439e6c1ac8b..cbbc24dbaf0f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -168,7 +168,7 @@ struct intel_guc {
>   	/** @lrc_desc_pool: object allocated to hold the GuC LRC descriptor pool */
>   	struct i915_vma *lrc_desc_pool;
>   	/** @lrc_desc_pool_vaddr: contents of the GuC LRC descriptor pool */
> -	void *lrc_desc_pool_vaddr;
> +	struct iosys_map lrc_desc_pool_vaddr;
>   
>   	/**
>   	 * @context_lookup: used to resolve intel_context from guc_id, if a
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 9ec03234d2c2..84b17ded886a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -467,13 +467,14 @@ static u32 *get_wq_pointer(struct guc_process_desc *desc,
>   	return &__get_parent_scratch(ce)->wq[ce->parallel.guc.wqi_tail / sizeof(u32)];
>   }
>   
> -static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
> +static void __write_lrc_desc(struct intel_guc *guc, u32 index,
> +			     struct guc_lrc_desc *desc)
>   {
> -	struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;
> +	unsigned int size = sizeof(struct guc_lrc_desc);
>   
>   	GEM_BUG_ON(index >= GUC_MAX_CONTEXT_ID);
>   
> -	return &base[index];
> +	iosys_map_memcpy_to(&guc->lrc_desc_pool_vaddr, index * size, desc, size);
>   }
>   
>   static inline struct intel_context *__get_context(struct intel_guc *guc, u32 id)
> @@ -489,20 +490,28 @@ static int guc_lrc_desc_pool_create(struct intel_guc *guc)
>   {
>   	u32 size;
>   	int ret;
> +	void *addr;
>   
>   	size = PAGE_ALIGN(sizeof(struct guc_lrc_desc) *
>   			  GUC_MAX_CONTEXT_ID);
>   	ret = intel_guc_allocate_and_map_vma(guc, size, &guc->lrc_desc_pool,
> -					     (void **)&guc->lrc_desc_pool_vaddr);
> +					     &addr);
> +
>   	if (ret)
>   		return ret;
>   
> +	if (i915_gem_object_is_lmem(guc->lrc_desc_pool->obj))
> +		iosys_map_set_vaddr_iomem(&guc->lrc_desc_pool_vaddr,
> +					  (void __iomem *)addr);
> +	else
> +		iosys_map_set_vaddr(&guc->lrc_desc_pool_vaddr, addr);
> +
>   	return 0;
>   }
>   
>   static void guc_lrc_desc_pool_destroy(struct intel_guc *guc)
>   {
> -	guc->lrc_desc_pool_vaddr = NULL;
> +	iosys_map_clear(&guc->lrc_desc_pool_vaddr);
>   	i915_vma_unpin_and_release(&guc->lrc_desc_pool, I915_VMA_RELEASE_MAP);
>   }
>   
> @@ -513,9 +522,11 @@ static inline bool guc_submission_initialized(struct intel_guc *guc)
>   
>   static inline void _reset_lrc_desc(struct intel_guc *guc, u32 id)
>   {
> -	struct guc_lrc_desc *desc = __get_lrc_desc(guc, id);
> +	unsigned int size = sizeof(struct guc_lrc_desc);
>   
> -	memset(desc, 0, sizeof(*desc));
> +	GEM_BUG_ON(id >= GUC_MAX_CONTEXT_ID);
> +
> +	iosys_map_memset(&guc->lrc_desc_pool_vaddr, id * size, 0, size);
>   }
>   
>   static inline bool ctx_id_mapped(struct intel_guc *guc, u32 id)
> @@ -2233,7 +2244,7 @@ static void prepare_context_registration_info(struct intel_context *ce)
>   	struct intel_engine_cs *engine = ce->engine;
>   	struct intel_guc *guc = &engine->gt->uc.guc;
>   	u32 ctx_id = ce->guc_id.id;
> -	struct guc_lrc_desc *desc;
> +	struct guc_lrc_desc desc;
>   	struct intel_context *child;
>   
>   	GEM_BUG_ON(!engine->mask);
> @@ -2245,13 +2256,13 @@ static void prepare_context_registration_info(struct intel_context *ce)
>   	GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=
>   		   i915_gem_object_is_lmem(ce->ring->vma->obj));
>   
> -	desc = __get_lrc_desc(guc, ctx_id);
> -	desc->engine_class = engine_class_to_guc_class(engine->class);
> -	desc->engine_submit_mask = engine->logical_mask;
> -	desc->hw_context_desc = ce->lrc.lrca;
> -	desc->priority = ce->guc_state.prio;
> -	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> -	guc_context_policy_init(engine, desc);
> +	memset(&desc, 0, sizeof(desc));
> +	desc.engine_class = engine_class_to_guc_class(engine->class);
> +	desc.engine_submit_mask = engine->logical_mask;
> +	desc.hw_context_desc = ce->lrc.lrca;
> +	desc.priority = ce->guc_state.prio;
> +	desc.context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> +	guc_context_policy_init(engine, &desc);
>   
>   	/*
>   	 * If context is a parent, we need to register a process descriptor
> @@ -2259,36 +2270,41 @@ static void prepare_context_registration_info(struct intel_context *ce)
>   	 */
>   	if (intel_context_is_parent(ce)) {
>   		struct guc_process_desc *pdesc;
> +		struct guc_lrc_desc child_desc;
>   
>   		ce->parallel.guc.wqi_tail = 0;
>   		ce->parallel.guc.wqi_head = 0;
>   
> -		desc->process_desc = i915_ggtt_offset(ce->state) +
> +		desc.process_desc = i915_ggtt_offset(ce->state) +
>   			__get_parent_scratch_offset(ce);
> -		desc->wq_addr = i915_ggtt_offset(ce->state) +
> +		desc.wq_addr = i915_ggtt_offset(ce->state) +
>   			__get_wq_offset(ce);
> -		desc->wq_size = WQ_SIZE;
> +		desc.wq_size = WQ_SIZE;
>   
>   		pdesc = __get_process_desc(ce);
>   		memset(pdesc, 0, sizeof(*(pdesc)));
>   		pdesc->stage_id = ce->guc_id.id;
> -		pdesc->wq_base_addr = desc->wq_addr;
> -		pdesc->wq_size_bytes = desc->wq_size;
> +		pdesc->wq_base_addr = desc.wq_addr;
> +		pdesc->wq_size_bytes = desc.wq_size;
>   		pdesc->wq_status = WQ_STATUS_ACTIVE;
>   
>   		for_each_child(ce, child) {
> -			desc = __get_lrc_desc(guc, child->guc_id.id);
> +			memset(&child_desc, 0, sizeof(child_desc));
>   
> -			desc->engine_class =
> +			child_desc.engine_class =
>   				engine_class_to_guc_class(engine->class);
> -			desc->hw_context_desc = child->lrc.lrca;
> -			desc->priority = ce->guc_state.prio;
> -			desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> -			guc_context_policy_init(engine, desc);
> +			child_desc.hw_context_desc = child->lrc.lrca;
> +			child_desc.priority = ce->guc_state.prio;
> +			child_desc.context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> +			guc_context_policy_init(engine, &child_desc);
> +
> +			__write_lrc_desc(guc, child->guc_id.id, &child_desc);
>   		}
>   
>   		clear_children_join_go_memory(ce);
>   	}
> +
> +	__write_lrc_desc(guc, ctx_id, &desc);
>   }
>   
>   static int try_context_registration(struct intel_context *ce, bool loop)