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

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

On 7/12/2023 5:39 PM, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Nirmoy Das <nirmoy.das@linux.intel.com>
> Sent: Wednesday, July 12, 2023 7:18 AM
> To: Andi Shyti <andi.shyti@linux.intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: Intel GFX <intel-gfx@lists.freedesktop.org>; Roper, Matthew D <matthew.d.roper@intel.com>; Chris Wilson <chris@chris-wilson.co.uk>; Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 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 <jonathan.cavitt@intel.com>
>>>
>>> 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 <jonathan.cavitt@intel.com>
>>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
>>> Cc: <stable@vger.kernel.org> # 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.

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.


Thanks,

Nirmoy

> -Jonathan Cavitt
>
>> Regards,
>>
>> Nirmoy
>>
>>
>>> +
>>>    	if (mode & EMIT_FLUSH) {
>>>    		u32 flags = 0;
>>>    		int err;