[Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3

[Nirmoy Das]

On 10/17/2023 2:23 AM, Belgaumkar, Vinay wrote:
>
> On 10/16/2023 4:24 PM, John Harrison wrote:
>> On 10/16/2023 15:55, Vinay Belgaumkar wrote:
>>> This bit does not cause an explicit L3 flush. We already use
>> At all? Or only on newer hardware? And as a genuine spec change or as 
>> a bug / workaround?
>>
>> If the hardware has re-purposed the bit then it is probably worth at 
>> least adding a comment to the bit definition to say that it is only 
>> valid up to IP version 12.70.
> At this point, this is a bug on MTL since this bit is not related to 
> L3 flushes as per spec. Regarding older platforms, still checking the 
> reason why this was added (i.e if it fixed something and will regress 
> if removed). If not, we can extend the change for others as well in a 
> separate patch. On older platforms, this bit seems to cause an 
> implicit flush at best.
>>
>>> PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose.
>>>
>>> Cc: Nirmoy Das <nirmoy.das at intel.com>
>>> Cc: Mikka Kuoppala <mika.kuoppala at intel.com>
>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
>>> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> index ba4c2422b340..abbc02f3e66e 100644
>>> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> @@ -247,6 +247,7 @@ static int mtl_dummy_pipe_control(struct 
>>> i915_request *rq)
>>>   int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>>>   {
>>>       struct intel_engine_cs *engine = rq->engine;
>>> +    struct intel_gt *gt = rq->engine->gt;
>>>         /*
>>>        * On Aux CCS platforms the invalidation of the Aux
>>> @@ -278,7 +279,8 @@ int gen12_emit_flush_rcs(struct i915_request 
>>> *rq, u32 mode)
>>>            * deals with Protected Memory which is not needed for
>>>            * AUX CCS invalidation and lead to unwanted side effects.
>>>            */
>>> -        if (mode & EMIT_FLUSH)
>>> +        if ((mode & EMIT_FLUSH) &&
>>> +            !(IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))))
>> Why stop at 12.71? Is the meaning only changed for 12.70 and the 
>> old/correct version will be restored in later hardware?
>
> Was trying to keep this limited to MTL for now until the above 
> statements are verified.

If we don't need it for MTL, we are most likely not going to need it 
after MTL too. It should be fine if you make it >= MTL until

we get more clarity about lower platforms.


With that,  this is

Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>

>
> Thanks,
>
> Vinay.
>
>>
>> John.
>>
>>
>>>               bit_group_1 |= PIPE_CONTROL_FLUSH_L3;
>>>             bit_group_1 |= PIPE_CONTROL_TILE_CACHE_FLUSH;
>>> @@ -812,12 +814,14 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct 
>>> i915_request *rq, u32 *cs)
>>>       u32 flags = (PIPE_CONTROL_CS_STALL |
>>>                PIPE_CONTROL_TLB_INVALIDATE |
>>>                PIPE_CONTROL_TILE_CACHE_FLUSH |
>>> -             PIPE_CONTROL_FLUSH_L3 |
>>>                PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
>>>                PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>>>                PIPE_CONTROL_DC_FLUSH_ENABLE |
>>>                PIPE_CONTROL_FLUSH_ENABLE);
>>>   +    if (!(IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))))
>>> +        flags |= PIPE_CONTROL_FLUSH_L3;
>>> +
>>>       /* Wa_14016712196 */
>>>       if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || 
>>> IS_DG2(i915))
>>>           /* dummy PIPE_CONTROL + depth flush */
>>