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

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

On 7/13/2023 4:23 PM, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Nirmoy Das <nirmoy.das@linux.intel.com>
> Sent: Thursday, July 13, 2023 7:12 AM
> To: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Cavitt, Jonathan <jonathan.cavitt@intel.com>; 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,
>>
>> 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?

It does actually, was not very clear from 1st look.


>
>
>>>>
>>>>> 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.

CCS Flush is new and I see a new bit for that in the gfxspec.

With that added, the patch looks good to me.


Thanks for your patience, Jonathan!


Nirmoy


Regards,

Nirmoy

> -Jonathan Cavitt
>
>
>>
>> Regards,
>>
>> Nirmoy
>>
>>> Andi