Re: [Intel-gfx] [PATCH v2 2/4] drm/i915/gt: Ensure memory quiesced before invalidation

[Nirmoy Das <nirmoy.das@linux.intel.com>]

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