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

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Tue, Mar 08, 2022 at 10:17:42PM +0530, 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@intel.com>
>Cc: John Harrison <John.C.Harrison@Intel.com>
>Cc: Matthew Brost <matthew.brost@intel.com>
>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>---
> 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;

s/_vaddr/_map/ for consistency with intel_guc_ads

>
> 	/**
> 	 * @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);

you are not using size anywhere else, so it would be preferred to keep the size
calculation inside this call.

	iosys_map_memcpy_to(&guc->lrc_desc_pool_vaddr, index * size, desc, sizeof(*desc));

which also avoids accidentally using the wrong struct if we ever change
the type of what we are copying.

> }
>
> 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;

vaddr for consistency

>
> 	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);

ditto. And maybe move it be close to __write_lrc_desc. I don't really
understand the difference here with 1 underscore vs the 2. Maybe as a
follow up just reconcile them?

The rest I left to another reply to focus on what may be the only
real issue I see in this patch and to get feedback from other people.

thanks
Lucas De Marchi