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

[Belgaumkar, Vinay]

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.

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 */
>