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;
/** * @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)
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
...
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -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));
previously we would re-use whatever was left in guc->lrc_desc_pool_vaddr. Here we are changing it to always zero everything and set the fields we are interested in.
As I'm not too familiar with this part and I see us traversing child guc_process_desc which may point to the same id, it doesn't _feel_ safe. Did you check if this is not zero'ing what it shouldn't?
Matt Brost / John / Daniele, could you clarify?
thanks Lucas De Marchi
On 11.03.2022 10:40, Lucas De Marchi wrote:
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
...
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -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));
previously we would re-use whatever was left in guc->lrc_desc_pool_vaddr. Here we are changing it to always zero everything and set the fields we are interested in.
As I'm not too familiar with this part and I see us traversing child guc_process_desc which may point to the same id, it doesn't _feel_ safe. Did you check if this is not zero'ing what it shouldn't?
Matt Brost / John / Daniele, could you clarify?
thanks Lucas De Marchi
I verified that struct guc_lrc_desc is not updated anywhere else in the driver other than in prepare_context_registration_info. So I went ahead with clearing it before updating the fields. But I will still wait for comments from Matt Brost/ John / Daniele for their confirmation.
Thanks Bala
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
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@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;
/**
- @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)
On Wed, Mar 30, 2022 at 08:53:11AM -0700, John Harrison wrote:
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.
We're shiping guc now (on dg1, and also some of the integrated already too), which means upgrading guc versions will break users and cause regressions, and that's a no-go.
So unless that v70 upgrade is exclusively for dg2 or another platform where enabling is still in the very early stages (i.e. the driver is unusable for booting to desktop) ... how does this work?
Or do I misunderstand something here? -Daniel
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@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; /**
- @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;
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->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;
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);- __write_lrc_desc(guc, ctx_id, &desc); } static int try_context_registration(struct intel_context *ce, bool loop)
dri-devel@lists.freedesktop.org
-
Balasubramani Vivekanandan -
Daniel Vetter -
John Harrison -
Lucas De Marchi