| Message ID | 20230517132230.1102553-1-tejas.upadhyay@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] drm/i915/gt: Add workaround 14016712196 | expand |
Hi Tejas, On Wed, May 17, 2023 at 06:52:30PM +0530, Tejas Upadhyay wrote: > Wa_14016712196 implementation for mtl > > Bspec: 72197 > > V2: > - Fix kernel test robot warnings > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel.com/ > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> > --- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 41 ++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > index e1c76e5bfa82..737eb515544b 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -177,14 +177,38 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv > return cs; > } > > +static int mtl_dummy_pipe_control(struct i915_request *rq, u32 *cs) > +{ > + if (IS_ERR(cs)) > + return PTR_ERR(cs); > + cs = gen12_emit_pipe_control(cs, > + 0, > + PIPE_CONTROL_DEPTH_CACHE_FLUSH, > + LRC_PPHWSP_SCRATCH_ADDR); > + intel_ring_advance(rq, cs); > + > + return 0; > +} > + > int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > { > struct intel_engine_cs *engine = rq->engine; > + int err; > > if (mode & EMIT_FLUSH) { > u32 flags = 0; > u32 *cs; > > + /* Wa_14016712196 */ > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { > + /* dummy PIPE_CONTROL + depth flush */ > + cs = intel_ring_begin(rq, 6); > + err = mtl_dummy_pipe_control(rq, cs); > + if (err) > + return err; > + } > + > flags |= PIPE_CONTROL_TILE_CACHE_FLUSH; > flags |= PIPE_CONTROL_FLUSH_L3; > flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH; > @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > u32 flags = 0; > u32 *cs, count; > > + /* Wa_14016712196 */ > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { > + /* dummy PIPE_CONTROL + depth flush */ > + cs = intel_ring_begin(rq, 6); > + err = mtl_dummy_pipe_control(rq, cs); > + if (err) > + return err; > + } > + as the two blocks are basically identical, why not put both of them inside mtl_dummy_pipe_control() and you would basically have something like: /* Wa_14016712196 */ cs = mtl_dummy_pipe_control(rq, cs); if (IS_ERR(cs)) return PTR_ERR(cs); Besides, you check the err value from intel_ring_begin inside mtl_dummy_pipe_control(), which looks a bit weird to me. I'm wondering, though, if we can put both dummy pipe and real pipe in the same command and advance the ring only once at the end... nevermind. Andi
> -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Wednesday, May 31, 2023 2:36 AM > To: Upadhyay, Tejas <tejas.upadhyay@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/gt: Add workaround > 14016712196 > > Hi Tejas, > > On Wed, May 17, 2023 at 06:52:30PM +0530, Tejas Upadhyay wrote: > > Wa_14016712196 implementation for mtl > > > > Bspec: 72197 > > > > V2: > > - Fix kernel test robot warnings > > > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel. > > com/ > > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> > > --- > > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 41 > > ++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > index e1c76e5bfa82..737eb515544b 100644 > > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > @@ -177,14 +177,38 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt > *gt, u32 *cs, const i915_reg_t inv > > return cs; > > } > > > > +static int mtl_dummy_pipe_control(struct i915_request *rq, u32 *cs) { > > + if (IS_ERR(cs)) > > + return PTR_ERR(cs); > > + cs = gen12_emit_pipe_control(cs, > > + 0, > > + PIPE_CONTROL_DEPTH_CACHE_FLUSH, > > + LRC_PPHWSP_SCRATCH_ADDR); > > + intel_ring_advance(rq, cs); > > + > > + return 0; > > +} > > + > > int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) { > > struct intel_engine_cs *engine = rq->engine; > > + int err; > > > > if (mode & EMIT_FLUSH) { > > u32 flags = 0; > > u32 *cs; > > > > + /* Wa_14016712196 */ > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, > STEP_B0) || > > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, > STEP_B0)) { > > + /* dummy PIPE_CONTROL + depth flush */ > > + cs = intel_ring_begin(rq, 6); > > + err = mtl_dummy_pipe_control(rq, cs); > > + if (err) > > + return err; > > + } > > + > > flags |= PIPE_CONTROL_TILE_CACHE_FLUSH; > > flags |= PIPE_CONTROL_FLUSH_L3; > > flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH; > > @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, > u32 mode) > > u32 flags = 0; > > u32 *cs, count; > > > > + /* Wa_14016712196 */ > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, > STEP_B0) || > > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, > STEP_B0)) { > > + /* dummy PIPE_CONTROL + depth flush */ > > + cs = intel_ring_begin(rq, 6); > > + err = mtl_dummy_pipe_control(rq, cs); > > + if (err) > > + return err; > > + } > > + > > as the two blocks are basically identical, why not put both of them inside > mtl_dummy_pipe_control() and you would basically have something like: > > /* Wa_14016712196 */ > cs = mtl_dummy_pipe_control(rq, cs); > if (IS_ERR(cs)) > return PTR_ERR(cs); > I agree initially I had same, but problem I have is kernel bot is complaining for uninitialized "cs" passed to mtl_dummy_pipe_control. I did not get simple solution to it. Suggestions! > Besides, you check the err value from intel_ring_begin inside > mtl_dummy_pipe_control(), which looks a bit weird to me. Yes it looks weird. I will change it. > > I'm wondering, though, if we can put both dummy pipe and real pipe in the > same command and advance the ring only once at the end... nevermind. As we have conditional ring increments, we cant decide ring size at start in ring begin for whole bunch at once. Though double checking. Thanks, Tejas > > Andi
Hi Tejas, > > > > I'm wondering, though, if we can put both dummy pipe and real pipe in the > > same command and advance the ring only once at the end... nevermind. > > As we have conditional ring increments, we cant decide ring size at start in ring begin for whole bunch at once. Though double checking. yes, you add it conditionally. Check, e.g., the control: if (!HAS_FLAT_CCS(rq->engine->i915)) count = 8 + 4; else count = 8; In the second if block. You are basically doing the same thing. But I'm not strong with this. Andi
On 17.05.2023 15:22, Tejas Upadhyay wrote: > Wa_14016712196 implementation for mtl > > Bspec: 72197 > > V2: > - Fix kernel test robot warnings > > Reported-by: kernel test robot <lkp@intel.com> I do not think robot reported lack of this wa :), putting lkp in changelog should be enough. > Closes: https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel.com/ > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> > --- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 41 ++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > index e1c76e5bfa82..737eb515544b 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -177,14 +177,38 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv > return cs; > } > > +static int mtl_dummy_pipe_control(struct i915_request *rq, u32 *cs) > +{ > + if (IS_ERR(cs)) > + return PTR_ERR(cs); > + cs = gen12_emit_pipe_control(cs, > + 0, > + PIPE_CONTROL_DEPTH_CACHE_FLUSH, > + LRC_PPHWSP_SCRATCH_ADDR); > + intel_ring_advance(rq, cs); > + > + return 0; > +} > + > int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > { > struct intel_engine_cs *engine = rq->engine; > + int err; > > if (mode & EMIT_FLUSH) { > u32 flags = 0; > u32 *cs; > > + /* Wa_14016712196 */ > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { > + /* dummy PIPE_CONTROL + depth flush */ > + cs = intel_ring_begin(rq, 6); > + err = mtl_dummy_pipe_control(rq, cs); > + if (err) > + return err; Like Andi suggested, you can replace above sequence with: cs = mtl_dummy_pipe_control(rq); > + } > + > flags |= PIPE_CONTROL_TILE_CACHE_FLUSH; > flags |= PIPE_CONTROL_FLUSH_L3; > flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH; > @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > u32 flags = 0; > u32 *cs, count; > > + /* Wa_14016712196 */ > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { > + /* dummy PIPE_CONTROL + depth flush */ > + cs = intel_ring_begin(rq, 6); > + err = mtl_dummy_pipe_control(rq, cs); > + if (err) > + return err; > + } > + In case gen12_emit_flush_rcs is called with EMIT_BARRIER = (EMIT_INVALIDATE | EMIT_FLUSH) dummy pipe will be inserted twice, is it OK? > flags |= PIPE_CONTROL_COMMAND_CACHE_INVALIDATE; > flags |= PIPE_CONTROL_TLB_INVALIDATE; > flags |= PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE; > @@ -733,6 +767,13 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs) > PIPE_CONTROL_DC_FLUSH_ENABLE | > PIPE_CONTROL_FLUSH_ENABLE); > > + /* Wa_14016712196 */ > + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0)) > + /* dummy PIPE_CONTROL + depth flush */ > + cs = gen12_emit_pipe_control(cs, 0, > + PIPE_CONTROL_DEPTH_CACHE_FLUSH, 0); How do we know there is enough space for dummy pipe? Regards Andrzej > + > if (GRAPHICS_VER(i915) == 12 && GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) > /* Wa_1409600907 */ > flags |= PIPE_CONTROL_DEPTH_STALL;
Hi Andrzej, > > @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > > u32 flags = 0; > > u32 *cs, count; > > + /* Wa_14016712196 */ > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { > > + /* dummy PIPE_CONTROL + depth flush */ > > + cs = intel_ring_begin(rq, 6); > > + err = mtl_dummy_pipe_control(rq, cs); > > + if (err) > > + return err; > > + } > > + > > In case gen12_emit_flush_rcs is called with EMIT_BARRIER = (EMIT_INVALIDATE > | EMIT_FLUSH) dummy pipe will be inserted twice, is it OK? good point... I also meant to ask this, indeed. I assumed that before the pipe control we needed the dummy pipe. Is that the case, Tejas? > > flags |= PIPE_CONTROL_COMMAND_CACHE_INVALIDATE; > > flags |= PIPE_CONTROL_TLB_INVALIDATE; > > flags |= PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE; > > @@ -733,6 +767,13 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs) > > PIPE_CONTROL_DC_FLUSH_ENABLE | > > PIPE_CONTROL_FLUSH_ENABLE); > > + /* Wa_14016712196 */ > > + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > > + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0)) > > + /* dummy PIPE_CONTROL + depth flush */ > > + cs = gen12_emit_pipe_control(cs, 0, > > + PIPE_CONTROL_DEPTH_CACHE_FLUSH, 0); > > How do we know there is enough space for dummy pipe? This should be ensured by intel_ring_begin(). Andi > Regards > Andrzej > > > + > > if (GRAPHICS_VER(i915) == 12 && GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) > > /* Wa_1409600907 */ > > flags |= PIPE_CONTROL_DEPTH_STALL;
> -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Wednesday, May 31, 2023 5:45 PM > To: Hajda, Andrzej <andrzej.hajda@intel.com> > Cc: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/gt: Add workaround > 14016712196 > > Hi Andrzej, > > > > @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request > *rq, u32 mode) > > > u32 flags = 0; > > > u32 *cs, count; > > > + /* Wa_14016712196 */ > > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, > STEP_B0) || > > > + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, > STEP_B0)) { > > > + /* dummy PIPE_CONTROL + depth flush */ > > > + cs = intel_ring_begin(rq, 6); > > > + err = mtl_dummy_pipe_control(rq, cs); > > > + if (err) > > > + return err; > > > + } > > > + > > > > In case gen12_emit_flush_rcs is called with EMIT_BARRIER = > > (EMIT_INVALIDATE > > | EMIT_FLUSH) dummy pipe will be inserted twice, is it OK? > > good point... I also meant to ask this, indeed. I assumed that before the pipe > control we needed the dummy pipe. Is that the case, Tejas? Yes before any DEPTH flush + pipe control we need dummy pipe control. Thanks, Tejas > > > > flags |= PIPE_CONTROL_COMMAND_CACHE_INVALIDATE; > > > flags |= PIPE_CONTROL_TLB_INVALIDATE; > > > flags |= PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE; > > > @@ -733,6 +767,13 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct > i915_request *rq, u32 *cs) > > > PIPE_CONTROL_DC_FLUSH_ENABLE | > > > PIPE_CONTROL_FLUSH_ENABLE); > > > + /* Wa_14016712196 */ > > > + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > > > + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0)) > > > + /* dummy PIPE_CONTROL + depth flush */ > > > + cs = gen12_emit_pipe_control(cs, 0, > > > + > PIPE_CONTROL_DEPTH_CACHE_FLUSH, 0); > > > > How do we know there is enough space for dummy pipe? > > This should be ensured by intel_ring_begin(). > > Andi > > > Regards > > Andrzej > > > > > + > > > if (GRAPHICS_VER(i915) == 12 && GRAPHICS_VER_FULL(i915) < > IP_VER(12, 50)) > > > /* Wa_1409600907 */ > > > flags |= PIPE_CONTROL_DEPTH_STALL;
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index e1c76e5bfa82..737eb515544b 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -177,14 +177,38 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv return cs; } +static int mtl_dummy_pipe_control(struct i915_request *rq, u32 *cs) +{ + if (IS_ERR(cs)) + return PTR_ERR(cs); + cs = gen12_emit_pipe_control(cs, + 0, + PIPE_CONTROL_DEPTH_CACHE_FLUSH, + LRC_PPHWSP_SCRATCH_ADDR); + intel_ring_advance(rq, cs); + + return 0; +} + int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) { struct intel_engine_cs *engine = rq->engine; + int err; if (mode & EMIT_FLUSH) { u32 flags = 0; u32 *cs; + /* Wa_14016712196 */ + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { + /* dummy PIPE_CONTROL + depth flush */ + cs = intel_ring_begin(rq, 6); + err = mtl_dummy_pipe_control(rq, cs); + if (err) + return err; + } + flags |= PIPE_CONTROL_TILE_CACHE_FLUSH; flags |= PIPE_CONTROL_FLUSH_L3; flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH; @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) u32 flags = 0; u32 *cs, count; + /* Wa_14016712196 */ + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || + IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) { + /* dummy PIPE_CONTROL + depth flush */ + cs = intel_ring_begin(rq, 6); + err = mtl_dummy_pipe_control(rq, cs); + if (err) + return err; + } + flags |= PIPE_CONTROL_COMMAND_CACHE_INVALIDATE; flags |= PIPE_CONTROL_TLB_INVALIDATE; flags |= PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE; @@ -733,6 +767,13 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs) PIPE_CONTROL_DC_FLUSH_ENABLE | PIPE_CONTROL_FLUSH_ENABLE); + /* Wa_14016712196 */ + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0)) + /* dummy PIPE_CONTROL + depth flush */ + cs = gen12_emit_pipe_control(cs, 0, + PIPE_CONTROL_DEPTH_CACHE_FLUSH, 0); + if (GRAPHICS_VER(i915) == 12 && GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) /* Wa_1409600907 */ flags |= PIPE_CONTROL_DEPTH_STALL;
Wa_14016712196 implementation for mtl Bspec: 72197 V2: - Fix kernel test robot warnings Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel.com/ Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 41 ++++++++++++++++++++++++ 1 file changed, 41 insertions(+)