| Message ID | 20231016225530.2152896-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3 | expand |
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. > PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose. > > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: Mikka Kuoppala <mika.kuoppala@intel.com> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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? 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 */
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@intel.com> >> Cc: Mikka Kuoppala <mika.kuoppala@intel.com> >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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 */ >
Vinay Belgaumkar <vinay.belgaumkar@intel.com> writes: > This bit does not cause an explicit L3 flush. We already use > PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose. > > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: Mikka Kuoppala <mika.kuoppala@intel.com> s/kk/k > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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)))) > bit_group_1 |= PIPE_CONTROL_FLUSH_L3; Yes its best to apply for MTL first. Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com< > > 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 */ > -- > 2.38.1
Hi Vinay, > > > 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@intel.com> > > > Cc: Mikka Kuoppala <mika.kuoppala@intel.com> > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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. I'm not fully conviced here... this is not what the hardware spec says. Am I reading the specs wrong? Is there any ongoing discussion with the hardware developers? Andi
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@intel.com> >>> Cc: Mikka Kuoppala <mika.kuoppala@intel.com> >>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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@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 */ >>
On 10/17/2023 4:42 PM, Andi Shyti wrote: > Hi Vinay, > >>>> 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@intel.com> >>>> Cc: Mikka Kuoppala <mika.kuoppala@intel.com> >>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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. > I'm not fully conviced here... this is not what the hardware spec > says. Am I reading the specs wrong? The main issue is we are using side-effects of that bit as to flush L3 but that is not it's primary task. Unless there is a WA specially mentioned for MTL to use that bit to flush L3, I see no reason to use on MTL or further. Regards, Nirmoy > > Is there any ongoing discussion with the hardware developers? > > Andi
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)))) 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 */
This bit does not cause an explicit L3 flush. We already use PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose. Cc: Nirmoy Das <nirmoy.das@intel.com> Cc: Mikka Kuoppala <mika.kuoppala@intel.com> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)