Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
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
Cc: Mikka Kuoppala
Signed-off-by: Vinay Belgaumkar
---
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
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
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
Cc: Mikka Kuoppala
Signed-off-by: Vinay Belgaumkar
---
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
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 */
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
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
> > > Cc: Mikka Kuoppala
> > > Signed-off-by: Vinay Belgaumkar
> > > ---
> > > 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
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
Vinay Belgaumkar writes:
> This bit does not cause an explicit L3 flush. We already use
> PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose.
>
> Cc: Nirmoy Das
> Cc: Mikka Kuoppala
s/kk/k
> Signed-off-by: Vinay Belgaumkar
> ---
> 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
> 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
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
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
Cc: Mikka Kuoppala
Signed-off-by: Vinay Belgaumkar
---
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 */
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
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
Cc: Mikka Kuoppala
Signed-off-by: Vinay Belgaumkar
---
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 */
[Intel-gfx] [PATCH] drm/i915/mtl: Don't set PIPE_CONTROL_FLUSH_L3
This bit does not cause an explicit L3 flush. We already use
PIPE_CONTROL_DC_FLUSH_ENABLE for that purpose.
Cc: Nirmoy Das
Cc: Mikka Kuoppala
Signed-off-by: Vinay Belgaumkar
---
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;
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
