-----Original Message-----
From: Nirmoy Das <nirmoy.das@xxxxxxxxxxxxxxx>
Sent: Thursday, July 13, 2023 7:12 AM
To: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Cc: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; Intel GFX <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH v2 2/4] drm/i915/gt: Ensure memory quiesced before invalidation
>
>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.
That's about my stance on it as well. I'd defer the decision to Nirmoy here.
>
>
>> Or, I can queue up a patch 5 adding this "pedantic" check and we
>> can discuss it separately.
I wouldn't offer much in terms of the discussion, unfortunately, so I'm fairly certain the
only thing that would come from that is a series of questions asking why the patch
wasn't squashed with this one to begin with.
>>
>>>>>> + */
>>>>>> + 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.
Does PIPE_CONTROL_FLUSH_L3 not do this?
>>>
>>>
>>>> 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 ?
That would certainly explain why there's no flag for it, if performing the CCS Flush
is a part of the EMIT_FLUSH pipeline by default.
-Jonathan Cavitt
>
>
>Regards,
>
>Nirmoy
>
>>
>> Andi
>
