On Wed, May 26, 2021 at 07:43:02PM -0700, Matthew Brost wrote:
> On Wed, May 26, 2021 at 06:34:44PM -0700, Daniele Ceraolo Spurio wrote:
> >
> >
> > On 5/26/2021 12:11 PM, Matthew Brost wrote:
> > > With virtual engines, it is no longer possible to know which specific
> > > physical engine a given request will be executed on at the time that
> > > request is generated. This means that the request itself must be engine
> > > agnostic - any direct register writes must be relative to the engine
> > > and not absolute addresses.
> > >
> > > The LRI command has support for engine relative addressing. However,
> > > the mechanism is not transparent to the driver. The scheme for Gen11
> > > (MI_LRI_ADD_CS_MMIO_START) requires the LRI address to have no
> > > absolute engine base component in the ring and BBs. The hardware then
> > > adds on the correct engine offset at execution time. This differs
> > > slightly for LRC where the upper bits of the base component are just
> > > ignored.
> > >
> > > Due to the non-trivial and differing schemes on different hardware, it
> > > is not possible to simply update the code that creates the LRI
> > > commands to set a remap flag and let the hardware get on with it.
> > > Instead, this patch adds function wrappers for generating the LRI
> > > command itself and then for constructing the correct address to use
> > > with the LRI.
> > >
> > > Bspec: 45606
> > > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > CC: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > CC: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > CC: Chris P Wilson <chris.p.wilson@xxxxxxxxx>
> > > CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 ++++---
> > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 22 ++++++++++++++++++++
> > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 +++
> > > drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 6 ++++++
> > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +---
> > > 5 files changed, 36 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > index 188dee13e017..a8a195bfcb57 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > @@ -1211,7 +1211,7 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> > > {
> > > struct i915_address_space *vm = rq->context->vm;
> > > struct intel_engine_cs *engine = rq->engine;
> > > - u32 base = engine->mmio_base;
> > > + u32 base = engine->lri_mmio_base;
> > > u32 *cs;
> > > int i;
> > > @@ -1223,7 +1223,7 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> > > if (IS_ERR(cs))
> > > return PTR_ERR(cs);
> > > - *cs++ = MI_LOAD_REGISTER_IMM(2);
> > > + *cs++ = MI_LOAD_REGISTER_IMM_REL(engine, 2);
> >
> > This is the only place where you changed the behavior and I think it is
> > going away
> > (https://lists.freedesktop.org/archives/dri-devel/2021-May/305328.html), so
> > the new macro is potentially not needed.
> >
>
> See my last comment, I think this irrelevant as I think I missed some
> cases where this macro should be used.
>
Actually this wrong, the macro is indeed used in all the places it is needed.
Let me talk to Jason tomororw about when he expects his series to land, I
suspect it is going to take a bit as IGTs have to updated as well. If GuC
virtual engines land before his series we need this. Even his series lands first
adding this macro + hooks isn't a terrible idea.
Matt
> > > *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(base, 0));
> > > *cs++ = upper_32_bits(pd_daddr);
> > > @@ -1245,7 +1245,8 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> > > if (IS_ERR(cs))
> > > return PTR_ERR(cs);
> > > - *cs++ = MI_LOAD_REGISTER_IMM(2 * GEN8_3LVL_PDPES) | MI_LRI_FORCE_POSTED;
> > > + *cs++ = MI_LOAD_REGISTER_IMM_REL(engine, 2 * GEN8_3LVL_PDPES) |
> > > + MI_LRI_FORCE_POSTED;
> > > for (i = GEN8_3LVL_PDPES; i--; ) {
> > > const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index 3f9a811eb02b..0de6bc533776 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -15,6 +15,7 @@
> > > #include "intel_engine_pm.h"
> > > #include "intel_engine_user.h"
> > > #include "intel_execlists_submission.h"
> > > +#include "intel_gpu_commands.h"
> > > #include "intel_gt.h"
> > > #include "intel_gt_requests.h"
> > > #include "intel_gt_pm.h"
> > > @@ -222,6 +223,25 @@ static u32 __engine_mmio_base(struct drm_i915_private *i915,
> > > return bases[i].base;
> > > }
> > > +static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
> > > +{
> > > + if (INTEL_GEN(engine->i915) < 11)
> > > + return false;
> > > +
> > > + return true;
> >
> > We already have intel_engine_has_relative_mmio(), can just re-use that. Note
> > that I915_ENGINE_HAS_RELATIVE_MMIO is only set for gen12+ at the moment;
> > this was because CI failed on ICL and since we urgently needed the change
> > for gen12 we just excluded gen11 and pushed (see Mika's comment @
> > https://lists.freedesktop.org/archives/intel-gfx/2019-September/211812.html).
> > It should be ok to extend that to gen11 if we get a green CI.
> >
>
> Let me send out a trybot with intel_engine_has_relative_mmio with this
> enabled for gen11.
>
> > > +}
> > > +
> > > +static void lri_init(struct intel_engine_cs *engine)
> > > +{
> > > + if (i915_engine_has_relative_lri(engine)) {
> > > + engine->lri_cmd_mode = MI_LRI_LRM_CS_MMIO;
> > > + engine->lri_mmio_base = 0;
> > > + } else {
> > > + engine->lri_cmd_mode = 0;
> > > + engine->lri_mmio_base = engine->mmio_base;
> > > + }
> > > +}
> > > +
> > > static void __sprint_engine_name(struct intel_engine_cs *engine)
> > > {
> > > /*
> > > @@ -329,6 +349,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> > > /* Nothing to do here, execute in order of dependencies */
> > > engine->schedule = NULL;
> > > + lri_init(engine);
> > > +
> > > ewma__engine_latency_init(&engine->latency);
> > > seqcount_init(&engine->stats.lock);
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > index 9ef349cd5cea..e48da23c9b0f 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -310,6 +310,9 @@ struct intel_engine_cs {
> > > u32 context_size;
> > > u32 mmio_base;
> > > + u32 lri_mmio_base;
> > > + u32 lri_cmd_mode;
> > > +
> > > /*
> > > * Some w/a require forcewake to be held (which prevents RC6) while
> > > * a particular engine is active. If so, we set fw_domain to which
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > > index 2694dbb9967e..f0f101134fd8 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > > @@ -134,8 +134,14 @@
> > > * simply ignores the register load under certain conditions.
> > > * - One can actually load arbitrary many arbitrary registers: Simply issue x
> > > * address/value pairs. Don't overdue it, though, x <= 2^4 must hold!
> > > + * - Newer hardware supports engine relative addressing but older hardware does
> > > + * not. This is required for hw engine load balancing. The
> > > + * MI_LOAD_REGISTER_IMM_REL macro can be used on both newer and older
> > > + * hardware.
> > > */
> > > #define MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
> > > +#define MI_LOAD_REGISTER_IMM_REL(egine, x) \
> > > + (MI_LOAD_REGISTER_IMM(x) | engine->lri_cmd_mode)
> >
> > This naming is a bit confusing, because MI_LOAD_REGISTER_IMM_REL is not
> > actually always relative so we also need to be careful of how we provide the
> > register values (i.e. with or without the mmio base). Also a bit worrying
> > for future proofing, since we'd need to make sure that any new CS register
> > access goes explicitly relative. Just my 2 cents, I know there was
> > contention on this patch in the past so I'm not going to jump in on the
> > fight :)
> >
>
> In the LRC the upper bits of the base is just ignored, while in ring it
> is added. It is a bit confusing but the comment message explains this.
>
> MI_LOAD_REGISTER_IMM_REL was Tvrtko's suggestion and I personally like
> it. It is total bikeshed how this should look, I'd say let's go with
> this and move on.
>
> > I have not checked if any of the other numerous instances of
> > MI_LOAD_REGISTER_IMM would benefit from going relative. I assume none is
> > strictly required, since otherwise virtual engines wouldn't work.
> >
>
> I did a quick browse of the driver and I think I am missing some
> instances. Let scrub the driver + fix this up with my next trybot
> attempt.
>
> Matt
>
> > Daniele
> >
> > > /* Gen11+. addr = base + (ctx_restore ? offset & GENMASK(12,2) : offset) */
> > > #define MI_LRI_LRM_CS_MMIO REG_BIT(19)
> > > #define MI_LRI_FORCE_POSTED (1<<12)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > index aafe2a4df496..390628666564 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > @@ -44,11 +44,9 @@ static void set_offsets(u32 *regs,
> > > flags = *data >> 6;
> > > data++;
> > > - *regs = MI_LOAD_REGISTER_IMM(count);
> > > + *regs = MI_LOAD_REGISTER_IMM_REL(engine, count);
> > > if (flags & POSTED)
> > > *regs |= MI_LRI_FORCE_POSTED;
> > > - if (INTEL_GEN(engine->i915) >= 11)
> > > - *regs |= MI_LRI_LRM_CS_MMIO;
> > > regs++;
> > > GEM_BUG_ON(!count);
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
