Thanks,
Oak
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Zeng,
> Oak
> Sent: August 24, 2023 4:38 PM
> To: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Cc: chris.p.wilson@xxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shyti, Andi
> <andi.shyti@xxxxxxxxx>; Das, Nirmoy <nirmoy.das@xxxxxxxxx>
> Subject: Re: [PATCH 1/3] drm/i915: Create a blitter context for GGTT
> updates
>
>
>
> Thanks,
> Oak
>
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> > Sent: August 24, 2023 11:52 AM
> > To: Zeng, Oak <oak.zeng@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shyti, Andi <andi.shyti@xxxxxxxxx>;
> > chris.p.wilson@xxxxxxxxxxxxxxx; Das, Nirmoy <nirmoy.das@xxxxxxxxx>
> > Subject: Re: [PATCH 1/3] drm/i915: Create a blitter context for GGTT
> > updates
> >
> > On Tue, Aug 22, 2023 at 11:28:57AM -0400, Oak Zeng wrote:
> > > From: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > >
> > > Create a separate blitter context if a platform requires
> > > GGTT updates using MI_UPDATE_GTT blitter command.
> > >
> > > Subsequent patch will introduce methods to update
> > > GGTT using this blitter context and MI_UPDATE_GTT blitter
> > > command.
> > >
> > > v2: Fix a typo in comment. (Oak)
> > >
> > > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > > Signed-off-by: Oak Zeng <oak.zeng@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++
> > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 44 +++++++++++++++++++-
> > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 ++
> > > drivers/gpu/drm/i915/gt/intel_gtt.c | 4 ++
> > > drivers/gpu/drm/i915/gt/intel_gtt.h | 2 +
> > > 5 files changed, 56 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..ee36db2fdaa7 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_BLIT 0x46
> > > +#define I915_GEM_HWS_GGTT_BLIT_ADDR
> > (I915_GEM_HWS_GGTT_BLIT * 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
> > > @@ -356,4 +358,6 @@ u64 intel_clamp_preempt_timeout_ms(struct
> > intel_engine_cs *engine, u64 value);
> > > u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64
> value);
> > > u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64
> > value);
> > >
> > > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready);
> > > +bool intel_engine_blitter_context_ready(struct intel_gt *gt);
> > > #endif /* _INTEL_RINGBUFFER_H_ */
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index dfb69fc977a0..d8c492a507a4 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -27,6 +27,7 @@
> > > #include "intel_gt_mcr.h"
> > > #include "intel_gt_pm.h"
> > > #include "intel_gt_requests.h"
> > > +#include "intel_gtt.h"
> > > #include "intel_lrc.h"
> > > #include "intel_lrc_reg.h"
> > > #include "intel_reset.h"
> > > @@ -1419,6 +1420,34 @@ void intel_engine_destroy_pinned_context(struct
> > intel_context *ce)
> > > intel_context_put(ce);
> > > }
> > >
> > > +void intel_engine_blitter_context_set_ready(struct intel_gt *gt, bool ready)
> > > +{
> > > + struct intel_engine_cs *engine = gt->engine[BCS0];
> > > +
> > > + if (engine && engine->blitter_context)
> > > + atomic_set(&engine->blitter_context_ready, ready ? 1 : 0);
> > > +}
> > > +
> > > +bool intel_engine_blitter_context_ready(struct intel_gt *gt)
> > > +{
> > > + struct intel_engine_cs *engine = gt->engine[BCS0];
> > > +
> > > + if (engine)
> > > + return atomic_read(&engine->blitter_context_ready) == 1;
> > > +
> > > + return false;
> > > +}
> > > +
> > > +static struct intel_context *
> > > +create_ggtt_blitter_context(struct intel_engine_cs *engine)
> > > +{
> > > + static struct lock_class_key kernel;
> > > +
> > > + /* MI_UPDATE_GTT can insert up to 512 PTE entries so get a bigger ring
> > */
> > > + return intel_engine_create_pinned_context(engine, engine->gt->vm,
> > SZ_512K,
> > > +
> > I915_GEM_HWS_GGTT_BLIT_ADDR,
> > > + &kernel, "ggtt_blitter_context");
> > > +}
> > > static struct intel_context *
> > > create_kernel_context(struct intel_engine_cs *engine)
> > > {
> > > @@ -1442,7 +1471,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 +1487,15 @@ 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 using blitter
> > > + * if a platform require such service.
> > > + */
> > > + if (i915_ggtt_require_blitter(engine->i915) && engine->id == BCS0) {
> > > + bce = create_ggtt_blitter_context(engine);
> > > + if (IS_ERR(bce))
> > > + return PTR_ERR(bce);
> > > + }
> > >
> > > ret = measure_breadcrumb_dw(ce);
> > > if (ret < 0)
> > > @@ -1465,6 +1503,7 @@ static int engine_init_common(struct
> intel_engine_cs
> > *engine)
> > >
> > > engine->emit_fini_breadcrumb_dw = ret;
> > > engine->kernel_context = ce;
> > > + engine->blitter_context = bce;
> > >
> > > return 0;
> > >
> > > @@ -1537,6 +1576,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->blitter_context)
> > > + intel_engine_destroy_pinned_context(engine->blitter_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 e99a6fa03d45..62095c0d8783 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -415,6 +415,9 @@ struct intel_engine_cs {
> > > struct llist_head barrier_tasks;
> > >
> > > struct intel_context *kernel_context; /* pinned */
> > > + struct intel_context *blitter_context; /* pinned, only for BCS0 */
> > > + /* mark the blitter engine's availability status */
> > > + atomic_t blitter_context_ready;
> > >
> > > /**
> > > * pinned_contexts_list: List of pinned contexts. This list is only
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > index 13944a14ea2d..9c77c97670fe 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > @@ -21,6 +21,10 @@
> > > #include "intel_gt_regs.h"
> > > #include "intel_gtt.h"
> > >
> > > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915)
> > > +{
> > > + return IS_METEORLAKE(i915);
> >
> > Drive-by comment: this workaround is tied to the Xe_LPM+ media IP, not
> > to the MTL platform. Other platforms that re-use Xe_LPM+ IP will also
> > be affected, whereas MTL platforms that lack media, or integrate a
> > different media chiplet will not be affected. So the condition here
> > should be:
> >
> > /* Wa_13010847436 */
> > return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
>
> Issue was observed on LNL A0 (fixed in A1), MTL.
>
> Not sure whether it is fixed on ARL or not.
>
> For LNL A0, there might be a different wa so this software wa is not needed.
> Double confirming.
>
> For now let's only enable MTL.
I just confirmed this HW bug is also on ARL. But ARL code is not ready yet. Will enable it once it is ready.
Oak
>
> >
> > But does this even belong in this patch? It sounds like patch #3 of the
> > series is where you intended to hook up this programming to the specific
> > workaround.
>
> This function is called from patch 2. So will keep it to patch 1.
>
> Oak
>
> >
> >
> > Matt
> >
> > > +}
> > >
> > > static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
> > > {
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > index 4d6296cdbcfd..9710eb031fb2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > > @@ -688,4 +688,6 @@ static inline struct sgt_dma {
> > > return (struct sgt_dma){ sg, addr, addr + sg_dma_len(sg) };
> > > }
> > >
> > > +bool i915_ggtt_require_blitter(struct drm_i915_private *i915);
> > > +
> > > #endif
> > > --
> > > 2.26.3
> > >
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
