Re: [Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Hi Nirmoy, [...] > > GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); > > cleanup_status_page(engine); > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h > > b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > index a7e677598004..a8f527fab0f0 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > @@ -416,6 +416,9 @@ struct intel_engine_cs { > > struct llist_head barrier_tasks; > > > > struct intel_context *kernel_context; /* pinned */ > > + struct intel_context *bind_context; /* pinned, only for BCS0 */ > > + /* mark the bind context's availability status */ > > + bool bind_context_ready; > > So... this is used only once, set to ready and move forward... > this would never be false, otherwise it means that we failed to > create the bind_context... > > > /** > > * pinned_contexts_list: List of pinned contexts. This list is only > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > > b/drivers/gpu/drm/i915/gt/intel_gt.c > > index 449f0b7fc843..cd0ff1597db9 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > > @@ -1019,3 +1019,21 @@ enum i915_map_type intel_gt_coherent_map_type(struct > > intel_gt *gt, > > else > > return I915_MAP_WC; > > } > > + > > +void intel_gt_bind_context_set_ready(struct intel_gt *gt, bool ready) > > +{ > > + struct intel_engine_cs *engine = gt->engine[BCS0]; > > + > > + if (engine && engine->bind_context) > > + engine->bind_context_ready = ready; > > +} > > + > > +bool intel_gt_is_bind_context_ready(struct intel_gt *gt) > > +{ > > + struct intel_engine_cs *engine = gt->engine[BCS0]; > > + > > + if (engine) > > + return engine->bind_context_ready; > > ... can't we just use the bind_context being NULL as a boolean > meaning and save this extra boolean that will always stay there > and always hold the same value? Please ignore this comment. Reading the next patches I see that I misunderstood this part... there are some toggles ready/not ready later on. Andi
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Hi Nirmoy, [...] > ce = create_kernel_context(engine); > if (IS_ERR(ce)) > return PTR_ERR(ce); > + /* > + * Create a separate pinned context for GGTT update with blitter engine > + * if a platform require such service. MI_UPDATE_GTT works on other > + * engines as well but BCS should be less busy engine so pick that for > + * GGTT updates. > + */ > + if (engine->id == BCS0) { > + bce = create_ggtt_bind_context(engine); > + if (IS_ERR(bce)) { > + intel_engine_destroy_pinned_context(ce); > + return PTR_ERR(bce); goto err_context. > + } > + } > > ret = measure_breadcrumb_dw(ce); > if (ret < 0) > @@ -1464,11 +1491,14 @@ static int engine_init_common(struct intel_engine_cs > *engine) > > engine->emit_fini_breadcrumb_dw = ret; > engine->kernel_context = ce; > + engine->bind_context = bce; > > return 0; > > err_context: > intel_engine_destroy_pinned_context(ce); > + if (bce) > + intel_engine_destroy_pinned_context(ce); You can make a goto err_bce_context here > return ret; > } > > @@ -1536,6 +1566,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs > *engine) > > if (engine->kernel_context) > intel_engine_destroy_pinned_context(engine->kernel_context); > + if (engine->bind_context) > + intel_engine_destroy_pinned_context(engine->bind_context); > + > blank line > GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); > cleanup_status_page(engine); > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h > b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index a7e677598004..a8f527fab0f0 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -416,6 +416,9 @@ struct intel_engine_cs { > struct llist_head barrier_tasks; > > struct intel_context *kernel_context; /* pinned */ > + struct intel_context *bind_context; /* pinned, only for BCS0 */ > + /* mark the bind context's availability status */ > + bool bind_context_ready; So... this is used only once, set to ready and move forward... this would never be false, otherwise it means that we failed to create the bind_context... > /** >* pinned_contexts_list: List of pinned contexts. This list is only > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > b/drivers/gpu/drm/i915/gt/intel_gt.c > index 449f0b7fc843..cd0ff1597db9 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -1019,3 +1019,21 @@ enum i915_map_type intel_gt_coherent_map_type(struct > intel_gt *gt, > else > return I915_MAP_WC; > } > + > +void intel_gt_bind_context_set_ready(struct intel_gt *gt, bool ready) > +{ > + struct intel_engine_cs *engine = gt->engine[BCS0]; > + > + if (engine && engine->bind_context) > + engine->bind_context_ready = ready; > +} > + > +bool intel_gt_is_bind_context_ready(struct intel_gt *gt) > +{ > + struct intel_engine_cs *engine = gt->engine[BCS0]; > + > + if (engine) > + return engine->bind_context_ready; ... can't we just use the bind_context being NULL as a boolean meaning and save this extra boolean that will always stay there and always hold the same value? > + return false; > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h > b/drivers/gpu/drm/i915/gt/intel_gt.h > index 239848bcb2a4..9e70e625cebc 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h > @@ -180,4 +180,6 @@ enum i915_map_type intel_gt_coherent_map_type(struct > intel_gt *gt, > struct drm_i915_gem_object *obj, > bool always_coherent); > > +void intel_gt_bind_context_set_ready(struct intel_gt *gt, bool ready); > +bool intel_gt_is_bind_context_ready(struct intel_gt *gt); I think there is no mention anywhere of these two functions. Can we add some comments or some description in the log? Thanks, Andi > #endif /* __INTEL_GT_H__ */ > -- > 2.41.0
[Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Create a separate kernel context if a platform requires GGTT updates using MI_UPDATE_GTT blitter command. Subsequent patch will introduce methods to update GGTT using this bind context and MI_UPDATE_GTT blitter command. Signed-off-by: Nirmoy Das Reviewed-by: Oak Zeng --- drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_cs.c| 35 +++- drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ drivers/gpu/drm/i915/gt/intel_gt.c | 18 ++ drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ 5 files changed, 59 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index b58c30ac8ef0..40269e4c1e31 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value) #define I915_GEM_HWS_SEQNO 0x40 #define I915_GEM_HWS_SEQNO_ADDR(I915_GEM_HWS_SEQNO * sizeof(u32)) #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) +#define I915_GEM_HWS_GGTT_BIND 0x46 +#define I915_GEM_HWS_GGTT_BIND_ADDR(I915_GEM_HWS_GGTT_BIND * sizeof(u32)) #define I915_GEM_HWS_PXP 0x60 #define I915_GEM_HWS_PXP_ADDR (I915_GEM_HWS_PXP * sizeof(u32)) #define I915_GEM_HWS_GSC 0x62 diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 84a75c95f3f7..0a58fe812ec5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1418,6 +1418,20 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce) intel_context_put(ce); } +static struct intel_context * +create_ggtt_bind_context(struct intel_engine_cs *engine) +{ + static struct lock_class_key kernel; + + /* +* MI_UPDATE_GTT can insert up to 512 PTE entries and there could be multiple +* bind requets at a time so get a bigger ring. +*/ + return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K, + I915_GEM_HWS_GGTT_BIND_ADDR, + &kernel, "ggtt_bind_context"); +} + static struct intel_context * create_kernel_context(struct intel_engine_cs *engine) { @@ -1441,7 +1455,7 @@ create_kernel_context(struct intel_engine_cs *engine) */ static int engine_init_common(struct intel_engine_cs *engine) { - struct intel_context *ce; + struct intel_context *ce, *bce = NULL; int ret; engine->set_default_submission(engine); @@ -1457,6 +1471,19 @@ static int engine_init_common(struct intel_engine_cs *engine) ce = create_kernel_context(engine); if (IS_ERR(ce)) return PTR_ERR(ce); + /* +* Create a separate pinned context for GGTT update with blitter engine +* if a platform require such service. MI_UPDATE_GTT works on other +* engines as well but BCS should be less busy engine so pick that for +* GGTT updates. +*/ + if (engine->id == BCS0) { + bce = create_ggtt_bind_context(engine); + if (IS_ERR(bce)) { + intel_engine_destroy_pinned_context(ce); + return PTR_ERR(bce); + } + } ret = measure_breadcrumb_dw(ce); if (ret < 0) @@ -1464,11 +1491,14 @@ static int engine_init_common(struct intel_engine_cs *engine) engine->emit_fini_breadcrumb_dw = ret; engine->kernel_context = ce; + engine->bind_context = bce; return 0; err_context: intel_engine_destroy_pinned_context(ce); + if (bce) + intel_engine_destroy_pinned_context(ce); return ret; } @@ -1536,6 +1566,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine) if (engine->kernel_context) intel_engine_destroy_pinned_context(engine->kernel_context); + if (engine->bind_context) + intel_engine_destroy_pinned_context(engine->bind_context); + GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); cleanup_status_page(engine); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a7e677598004..a8f527fab0f0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -416,6 +416,9 @@ struct intel_engine_cs { struct llist_head barrier_tasks; struct intel_context *kernel_context; /* pinned */ + struct intel_context *bind_context; /* pinned, only for BCS0 */ + /* mark the bind context's availability status */ + bool bind_context_ready; /** * pinned_contexts_list: List of pinned contexts. This list is only diff --git a/drivers/gpu/
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Reviewed-by: Oak Zeng Thanks, Oak > -Original Message- > From: Das, Nirmoy > Sent: Friday, September 15, 2023 4:34 AM > To: intel-gfx@lists.freedesktop.org > Cc: Zeng, Oak ; chris.p.wil...@linux.intel.com; > Piorkowski, > Piotr ; Shyti, Andi ; Mun, > Gwan-gyeong ; Roper, Matthew D > ; Das, Nirmoy > Subject: [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates > > Create a separate kernel context if a platform requires > GGTT updates using MI_UPDATE_GTT blitter command. > > Subsequent patch will introduce methods to update > GGTT using this bind context and MI_UPDATE_GTT blitter > command. > > Signed-off-by: Nirmoy Das > --- > drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ > drivers/gpu/drm/i915/gt/intel_engine_cs.c| 35 +++- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ > drivers/gpu/drm/i915/gt/intel_gt.c | 18 ++ > drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ > 5 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h > b/drivers/gpu/drm/i915/gt/intel_engine.h > index b58c30ac8ef0..40269e4c1e31 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, > int reg, u32 value) > #define I915_GEM_HWS_SEQNO 0x40 > #define I915_GEM_HWS_SEQNO_ADDR (I915_GEM_HWS_SEQNO > * sizeof(u32)) > #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) > +#define I915_GEM_HWS_GGTT_BIND 0x46 > +#define I915_GEM_HWS_GGTT_BIND_ADDR (I915_GEM_HWS_GGTT_BIND * > sizeof(u32)) > #define I915_GEM_HWS_PXP 0x60 > #define I915_GEM_HWS_PXP_ADDR(I915_GEM_HWS_PXP * > sizeof(u32)) > #define I915_GEM_HWS_GSC 0x62 > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index dfb69fc977a0..18ae56ea012a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -1419,6 +1419,20 @@ void intel_engine_destroy_pinned_context(struct > intel_context *ce) > intel_context_put(ce); > } > > +static struct intel_context * > +create_ggtt_bind_context(struct intel_engine_cs *engine) > +{ > + static struct lock_class_key kernel; > + > + /* > + * MI_UPDATE_GTT can insert up to 512 PTE entries and there could be > multiple > + * bind requets at a time so get a bigger ring. > + */ > + return intel_engine_create_pinned_context(engine, engine->gt->vm, > SZ_512K, > + > I915_GEM_HWS_GGTT_BIND_ADDR, > + &kernel, "ggtt_bind_context"); > +} > + > static struct intel_context * > create_kernel_context(struct intel_engine_cs *engine) > { > @@ -1442,7 +1456,7 @@ create_kernel_context(struct intel_engine_cs *engine) > */ > static int engine_init_common(struct intel_engine_cs *engine) > { > - struct intel_context *ce; > + struct intel_context *ce, *bce = NULL; > int ret; > > engine->set_default_submission(engine); > @@ -1458,6 +1472,19 @@ static int engine_init_common(struct intel_engine_cs > *engine) > ce = create_kernel_context(engine); > if (IS_ERR(ce)) > return PTR_ERR(ce); > + /* > + * Create a separate pinned context for GGTT update with blitter engine > + * if a platform require such service. MI_UPDATE_GTT works on other > + * engines as well but BCS should be less busy engine so pick that for > + * GGTT updates. > + */ > + if (engine->id == BCS0) { > + bce = create_ggtt_bind_context(engine); > + if (IS_ERR(bce)) { > + intel_engine_destroy_pinned_context(ce); > + return PTR_ERR(bce); > + } > + } > > ret = measure_breadcrumb_dw(ce); > if (ret < 0) > @@ -1465,11 +1492,14 @@ static int engine_init_common(struct intel_engine_cs > *engine) > > engine->emit_fini_breadcrumb_dw = ret; > engine->kernel_context = ce; > + engine->bind_context = bce; > > return 0; > > err_context: > intel_engine_destroy_pinned_context(ce); > + if (bce) > + intel_engine_destroy_pinned_context(ce); > return ret; > } > > @@ -1537,6 +1567,9 @@ void intel_engine_cleanup_common(struct > intel_engine_cs *engine) > > if (engine->kernel_context) > intel_engine_destroy_pinned_context(engine->kernel_context); > + if (engine->bind_context) > + intel_engine_destroy_pinned_context(engine->bind_context); > + > > GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); > cleanup_status_page(engine); > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h > b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index a7e677598004..a8f527fab0f0 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/
[Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Create a separate kernel context if a platform requires GGTT updates using MI_UPDATE_GTT blitter command. Subsequent patch will introduce methods to update GGTT using this bind context and MI_UPDATE_GTT blitter command. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_cs.c| 35 +++- drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ drivers/gpu/drm/i915/gt/intel_gt.c | 18 ++ drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ 5 files changed, 59 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index b58c30ac8ef0..40269e4c1e31 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value) #define I915_GEM_HWS_SEQNO 0x40 #define I915_GEM_HWS_SEQNO_ADDR(I915_GEM_HWS_SEQNO * sizeof(u32)) #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) +#define I915_GEM_HWS_GGTT_BIND 0x46 +#define I915_GEM_HWS_GGTT_BIND_ADDR(I915_GEM_HWS_GGTT_BIND * sizeof(u32)) #define I915_GEM_HWS_PXP 0x60 #define I915_GEM_HWS_PXP_ADDR (I915_GEM_HWS_PXP * sizeof(u32)) #define I915_GEM_HWS_GSC 0x62 diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index dfb69fc977a0..18ae56ea012a 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1419,6 +1419,20 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce) intel_context_put(ce); } +static struct intel_context * +create_ggtt_bind_context(struct intel_engine_cs *engine) +{ + static struct lock_class_key kernel; + + /* +* MI_UPDATE_GTT can insert up to 512 PTE entries and there could be multiple +* bind requets at a time so get a bigger ring. +*/ + return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K, + I915_GEM_HWS_GGTT_BIND_ADDR, + &kernel, "ggtt_bind_context"); +} + static struct intel_context * create_kernel_context(struct intel_engine_cs *engine) { @@ -1442,7 +1456,7 @@ create_kernel_context(struct intel_engine_cs *engine) */ static int engine_init_common(struct intel_engine_cs *engine) { - struct intel_context *ce; + struct intel_context *ce, *bce = NULL; int ret; engine->set_default_submission(engine); @@ -1458,6 +1472,19 @@ static int engine_init_common(struct intel_engine_cs *engine) ce = create_kernel_context(engine); if (IS_ERR(ce)) return PTR_ERR(ce); + /* +* Create a separate pinned context for GGTT update with blitter engine +* if a platform require such service. MI_UPDATE_GTT works on other +* engines as well but BCS should be less busy engine so pick that for +* GGTT updates. +*/ + if (engine->id == BCS0) { + bce = create_ggtt_bind_context(engine); + if (IS_ERR(bce)) { + intel_engine_destroy_pinned_context(ce); + return PTR_ERR(bce); + } + } ret = measure_breadcrumb_dw(ce); if (ret < 0) @@ -1465,11 +1492,14 @@ static int engine_init_common(struct intel_engine_cs *engine) engine->emit_fini_breadcrumb_dw = ret; engine->kernel_context = ce; + engine->bind_context = bce; return 0; err_context: intel_engine_destroy_pinned_context(ce); + if (bce) + intel_engine_destroy_pinned_context(ce); return ret; } @@ -1537,6 +1567,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine) if (engine->kernel_context) intel_engine_destroy_pinned_context(engine->kernel_context); + if (engine->bind_context) + intel_engine_destroy_pinned_context(engine->bind_context); + GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); cleanup_status_page(engine); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a7e677598004..a8f527fab0f0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -416,6 +416,9 @@ struct intel_engine_cs { struct llist_head barrier_tasks; struct intel_context *kernel_context; /* pinned */ + struct intel_context *bind_context; /* pinned, only for BCS0 */ + /* mark the bind context's availability status */ + bool bind_context_ready; /** * pinned_contexts_list: List of pinned contexts. This list is only diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Hi Oak, On 9/13/2023 6:30 PM, Zeng, Oak wrote: Thanks, Oak -Original Message- From: Das, Nirmoy Sent: Wednesday, September 13, 2023 9:10 AM To: intel-gfx@lists.freedesktop.org Cc: Zeng, Oak ; chris.p.wil...@linux.intel.com; Piorkowski, Piotr ; Shyti, Andi ; Mun, Gwan-gyeong ; Roper, Matthew D ; Das, Nirmoy Subject: [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates Create a separate kernel context if a platform requires GGTT updates using MI_UPDATE_GTT blitter command. Subsequent patch will introduce methods to update GGTT using this bind context and MI_UPDATE_GTT blitter command. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_cs.c| 33 +++- drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ drivers/gpu/drm/i915/gt/intel_gt.c | 18 +++ drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ 5 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index b58c30ac8ef0..40269e4c1e31 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value) #define I915_GEM_HWS_SEQNO0x40 #define I915_GEM_HWS_SEQNO_ADDR (I915_GEM_HWS_SEQNO * sizeof(u32)) #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) +#define I915_GEM_HWS_GGTT_BIND 0x46 +#define I915_GEM_HWS_GGTT_BIND_ADDR(I915_GEM_HWS_GGTT_BIND * sizeof(u32)) #define I915_GEM_HWS_PXP 0x60 #define I915_GEM_HWS_PXP_ADDR (I915_GEM_HWS_PXP * sizeof(u32)) #define I915_GEM_HWS_GSC 0x62 diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index dfb69fc977a0..52a24f55cb57 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1419,6 +1419,20 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce) intel_context_put(ce); } +static struct intel_context * +create_ggtt_bind_context(struct intel_engine_cs *engine) +{ + static struct lock_class_key kernel; + + /* +* MI_UPDATE_GTT can insert up to 512 PTE entries and there could be multiple +* bind requets at a time so get a bigger ring. +*/ + return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K, + I915_GEM_HWS_GGTT_BIND_ADDR, + &kernel, "ggtt_bind_context"); +} + static struct intel_context * create_kernel_context(struct intel_engine_cs *engine) { @@ -1442,7 +1456,7 @@ create_kernel_context(struct intel_engine_cs *engine) */ static int engine_init_common(struct intel_engine_cs *engine) { - struct intel_context *ce; + struct intel_context *ce, *bce = NULL; int ret; engine->set_default_submission(engine); @@ -1458,6 +1472,17 @@ static int engine_init_common(struct intel_engine_cs *engine) ce = create_kernel_context(engine); if (IS_ERR(ce)) return PTR_ERR(ce); + /* +* Create a separate pinned context for GGTT update with blitter engine +* if a platform require such service. MI_UPDATE_GTT works on other +* engines as well but BCS should be less busy engine so pick that for +* GGTT updates. +*/ + if (engine->id == BCS0) { + bce = create_ggtt_bind_context(engine); + if (IS_ERR(bce)) + return PTR_ERR(bce); Do you need to destroy ce before return? Yes , I will fix it in next rev. Thanks, Nirmoy Oak + } ret = measure_breadcrumb_dw(ce); if (ret < 0) @@ -1465,11 +1490,14 @@ static int engine_init_common(struct intel_engine_cs *engine) engine->emit_fini_breadcrumb_dw = ret; engine->kernel_context = ce; + engine->bind_context = bce; return 0; err_context: intel_engine_destroy_pinned_context(ce); + if (bce) + intel_engine_destroy_pinned_context(ce); return ret; } @@ -1537,6 +1565,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine) if (engine->kernel_context) intel_engine_destroy_pinned_context(engine->kernel_context); + if (engine->bind_context) + intel_engine_destroy_pinned_context(engine->bind_context); + GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); cleanup_status_page(engine); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a7e677598004..a8f527fab0f0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -416,6 +416,9 @@ struct intel_engine_cs { struct llist_head barrier_tasks
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Thanks, Oak > -Original Message- > From: Das, Nirmoy > Sent: Wednesday, September 13, 2023 9:10 AM > To: intel-gfx@lists.freedesktop.org > Cc: Zeng, Oak ; chris.p.wil...@linux.intel.com; > Piorkowski, > Piotr ; Shyti, Andi ; Mun, > Gwan-gyeong ; Roper, Matthew D > ; Das, Nirmoy > Subject: [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates > > Create a separate kernel context if a platform requires > GGTT updates using MI_UPDATE_GTT blitter command. > > Subsequent patch will introduce methods to update > GGTT using this bind context and MI_UPDATE_GTT blitter > command. > > Signed-off-by: Nirmoy Das > --- > drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ > drivers/gpu/drm/i915/gt/intel_engine_cs.c| 33 +++- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ > drivers/gpu/drm/i915/gt/intel_gt.c | 18 +++ > drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ > 5 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h > b/drivers/gpu/drm/i915/gt/intel_engine.h > index b58c30ac8ef0..40269e4c1e31 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, > int reg, u32 value) > #define I915_GEM_HWS_SEQNO 0x40 > #define I915_GEM_HWS_SEQNO_ADDR (I915_GEM_HWS_SEQNO > * sizeof(u32)) > #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) > +#define I915_GEM_HWS_GGTT_BIND 0x46 > +#define I915_GEM_HWS_GGTT_BIND_ADDR (I915_GEM_HWS_GGTT_BIND * > sizeof(u32)) > #define I915_GEM_HWS_PXP 0x60 > #define I915_GEM_HWS_PXP_ADDR(I915_GEM_HWS_PXP * > sizeof(u32)) > #define I915_GEM_HWS_GSC 0x62 > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index dfb69fc977a0..52a24f55cb57 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -1419,6 +1419,20 @@ void intel_engine_destroy_pinned_context(struct > intel_context *ce) > intel_context_put(ce); > } > > +static struct intel_context * > +create_ggtt_bind_context(struct intel_engine_cs *engine) > +{ > + static struct lock_class_key kernel; > + > + /* > + * MI_UPDATE_GTT can insert up to 512 PTE entries and there could be > multiple > + * bind requets at a time so get a bigger ring. > + */ > + return intel_engine_create_pinned_context(engine, engine->gt->vm, > SZ_512K, > + > I915_GEM_HWS_GGTT_BIND_ADDR, > + &kernel, "ggtt_bind_context"); > +} > + > static struct intel_context * > create_kernel_context(struct intel_engine_cs *engine) > { > @@ -1442,7 +1456,7 @@ create_kernel_context(struct intel_engine_cs *engine) > */ > static int engine_init_common(struct intel_engine_cs *engine) > { > - struct intel_context *ce; > + struct intel_context *ce, *bce = NULL; > int ret; > > engine->set_default_submission(engine); > @@ -1458,6 +1472,17 @@ static int engine_init_common(struct intel_engine_cs > *engine) > ce = create_kernel_context(engine); > if (IS_ERR(ce)) > return PTR_ERR(ce); > + /* > + * Create a separate pinned context for GGTT update with blitter engine > + * if a platform require such service. MI_UPDATE_GTT works on other > + * engines as well but BCS should be less busy engine so pick that for > + * GGTT updates. > + */ > + if (engine->id == BCS0) { > + bce = create_ggtt_bind_context(engine); > + if (IS_ERR(bce)) > + return PTR_ERR(bce); Do you need to destroy ce before return? Oak > + } > > ret = measure_breadcrumb_dw(ce); > if (ret < 0) > @@ -1465,11 +1490,14 @@ static int engine_init_common(struct intel_engine_cs > *engine) > > engine->emit_fini_breadcrumb_dw = ret; > engine->kernel_context = ce; > + engine->bind_context = bce; > > return 0; > > err_context: > intel_engine_destroy_pinned_context(ce); > + if (bce) > + intel_engine_destroy_pinned_context(ce); > return ret; > } > > @@ -1537,6 +1565,9 @@ void intel_engine_cleanup_common(struct > intel_engine_cs *engine) > > if (engine->kernel_context) > intel_engine_destroy_pinned_context(engine->kernel_context); > + if (engine->bind_context) > + intel_engine_destroy_pinned_context(engine->bind_context); > + > > GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); > cleanup_status_page(engine); > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h > b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index a7e677598004..a8f527fab0f0 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -416,6 +416,
[Intel-gfx] [PATCH 2/7] drm/i915: Create a kernel context for GGTT updates
Create a separate kernel context if a platform requires GGTT updates using MI_UPDATE_GTT blitter command. Subsequent patch will introduce methods to update GGTT using this bind context and MI_UPDATE_GTT blitter command. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/i915/gt/intel_engine.h | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_cs.c| 33 +++- drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++ drivers/gpu/drm/i915/gt/intel_gt.c | 18 +++ drivers/gpu/drm/i915/gt/intel_gt.h | 2 ++ 5 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index b58c30ac8ef0..40269e4c1e31 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -170,6 +170,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value) #define I915_GEM_HWS_SEQNO 0x40 #define I915_GEM_HWS_SEQNO_ADDR(I915_GEM_HWS_SEQNO * sizeof(u32)) #define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32)) +#define I915_GEM_HWS_GGTT_BIND 0x46 +#define I915_GEM_HWS_GGTT_BIND_ADDR(I915_GEM_HWS_GGTT_BIND * sizeof(u32)) #define I915_GEM_HWS_PXP 0x60 #define I915_GEM_HWS_PXP_ADDR (I915_GEM_HWS_PXP * sizeof(u32)) #define I915_GEM_HWS_GSC 0x62 diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index dfb69fc977a0..52a24f55cb57 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1419,6 +1419,20 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce) intel_context_put(ce); } +static struct intel_context * +create_ggtt_bind_context(struct intel_engine_cs *engine) +{ + static struct lock_class_key kernel; + + /* +* MI_UPDATE_GTT can insert up to 512 PTE entries and there could be multiple +* bind requets at a time so get a bigger ring. +*/ + return intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_512K, + I915_GEM_HWS_GGTT_BIND_ADDR, + &kernel, "ggtt_bind_context"); +} + static struct intel_context * create_kernel_context(struct intel_engine_cs *engine) { @@ -1442,7 +1456,7 @@ create_kernel_context(struct intel_engine_cs *engine) */ static int engine_init_common(struct intel_engine_cs *engine) { - struct intel_context *ce; + struct intel_context *ce, *bce = NULL; int ret; engine->set_default_submission(engine); @@ -1458,6 +1472,17 @@ static int engine_init_common(struct intel_engine_cs *engine) ce = create_kernel_context(engine); if (IS_ERR(ce)) return PTR_ERR(ce); + /* +* Create a separate pinned context for GGTT update with blitter engine +* if a platform require such service. MI_UPDATE_GTT works on other +* engines as well but BCS should be less busy engine so pick that for +* GGTT updates. +*/ + if (engine->id == BCS0) { + bce = create_ggtt_bind_context(engine); + if (IS_ERR(bce)) + return PTR_ERR(bce); + } ret = measure_breadcrumb_dw(ce); if (ret < 0) @@ -1465,11 +1490,14 @@ static int engine_init_common(struct intel_engine_cs *engine) engine->emit_fini_breadcrumb_dw = ret; engine->kernel_context = ce; + engine->bind_context = bce; return 0; err_context: intel_engine_destroy_pinned_context(ce); + if (bce) + intel_engine_destroy_pinned_context(ce); return ret; } @@ -1537,6 +1565,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine) if (engine->kernel_context) intel_engine_destroy_pinned_context(engine->kernel_context); + if (engine->bind_context) + intel_engine_destroy_pinned_context(engine->bind_context); + GEM_BUG_ON(!llist_empty(&engine->barrier_tasks)); cleanup_status_page(engine); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a7e677598004..a8f527fab0f0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -416,6 +416,9 @@ struct intel_engine_cs { struct llist_head barrier_tasks; struct intel_context *kernel_context; /* pinned */ + struct intel_context *bind_context; /* pinned, only for BCS0 */ + /* mark the bind context's availability status */ + bool bind_context_ready; /** * pinned_contexts_list: List of pinned contexts. This list is only diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 449f0b7fc843..cd0ff1597db9 100644 --- a/