-----Original Message----- From: Nirmoy Das <[email protected]> Sent: Wednesday, July 12, 2023 7:18 AM To: Andi Shyti <[email protected]>; Cavitt, Jonathan <[email protected]> Cc: Intel GFX <[email protected]>; Roper, Matthew D <[email protected]>; Chris Wilson <[email protected]>; Mika Kuoppala <[email protected]> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915/gt: Ensure memory quiesced before invalidation > >Hi Andi and Jonathan, > >On 6/27/2023 11:43 AM, Andi Shyti wrote: >> From: Jonathan Cavitt <[email protected]> >> >> All memory traffic must be quiesced before requesting >> an aux invalidation on platforms that use Aux CCS. >> >> Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all >> engines") >> Signed-off-by: Jonathan Cavitt <[email protected]> >> Signed-off-by: Andi Shyti <[email protected]> >> Cc: <[email protected]> # v5.8+ >> --- >> drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c >> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c >> index 563efee055602..e10e1ad0e841f 100644 >> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c >> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c >> @@ -202,6 +202,13 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 >> mode) >> { >> struct intel_engine_cs *engine = rq->engine; >> >> + /* >> + * Aux invalidations on Aux CCS platforms require >> + * memory traffic is quiesced prior. > >I see that we are doing aux inval on EMIT_INVALIDATE so it make sense to > > do if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915) ) >
This is agreeable, though I don't think there's any instances of us calling gen12_emit_flush_rcs with a blank mode, since that wouldn't accomplish anything. So I don't think the additional check/safety net is necessary, but it doesn't hurt to have. >> + */ >> + if (!HAS_FLAT_CCS(engine->i915)) >> + mode |= EMIT_FLUSH; > >I think this generic EMIT_FLUSH is not enough. I seeing some missing >flags for PIPE_CONTROL > >As per https://gfxspecs.intel.com/Predator/Home/Index/43904. It makes >sense to move this to a > >new function given the complexity of PIPE_CONTROL flags requires for this. > I'm assuming when you're talking about the missing flags for PIPE_CONTROL, you're referring to CCS Flush, correct? Because every other flag is already covered in the EMIT_FLUSH path. I feel like I had this conversation with Matt while the internal version was developed back in February, and the consensus was that the CCS Flush requirement was already covered. On the other hand, it looks like the CCS Flush requirement was only recently added back in May, so it might be worth double-checking at the very least. Although... if CCS Flush is a missing flag, I wonder how we're supposed to set it, as there doesn’t appear to be a definition for such a flag in intel_gpu_commands.h... -Jonathan Cavitt > >Regards, > >Nirmoy > > >> + >> if (mode & EMIT_FLUSH) { >> u32 flags = 0; >> int err; >
