Hi Andi,
On 7/13/2023 2:31 PM, Andi Shyti wrote:
Hi Nirmoy and Jonathan,
@@ -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.
so... do we agree here that we don't add anything? I don't really
mind...
Not a blocking objection but if you are sending another revision of this
then why not.
Or, I can queue up a patch 5 adding this "pedantic" check and we
can discuss it separately.
+ */
+ 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.
Yes, CCS Flush and I don't see a L3 fabric flush as well.
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.
Wasn't aware of this, would be nice to have a confirmation and a comment so
we
don't get confused in future.
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...
Yes, not yet but we should add a flag for that.
Is it OK if I add in the comment that EMIT_FLUSH covers the CCS
flushing?
is it though ? I don't see that in the bspec, may be I am missing
something ?
Regards,
Nirmoy
Andi
