| Message ID | 20230513021438.185352-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A-step | expand |
On 5/13/2023 7:44 AM, Radhakrishna Sripada wrote: > The dg2 workaround which is used for performance tuning > is needed for Meteorlake A-step. > > v2: Limit the WA for A-step > > Bspec: 68331 > Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Gustavo Sousa <gustavo.sousa@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 81a96c52a92b..9c1007c44298 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct intel_context *ce, u32 *cs) > cs, GEN12_GFX_CCS_AUX_NV); > > /* Wa_16014892111 */ > - if (IS_DG2(ce->engine->i915)) > + if (IS_DG2(ce->engine->i915) || > + IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > cs = dg2_emit_draw_watermark_setting(cs); > looks good to me. Acked-by: Haridhar Kalvala <haridhar.kalvala@intel.com> > return cs;
Quoting Radhakrishna Sripada (2023-05-12 23:14:37) >The dg2 workaround which is used for performance tuning >is needed for Meteorlake A-step. > >v2: Limit the WA for A-step I think what Matt meant in the review for v1 was that this commit should be rather about the tuning setting rather than the workaround itself. As such, maybe we should change the commit message so that it focus on the recommended tuning setting, i.e., instead of "Extend Wa_16014892111 to MTL A-step" as subject, we should write something like "Apply recommended tuning setting for ..." and give details. That said, since we are focusing on the tuning settings here, I guess this could be squashed with the second patch and we could add a note about DRAW_WATERMARK needing Wa_16014892111 for A steps of MTL. -- Gustavo Sousa > >Bspec: 68331 >Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> >Cc: Matt Roper <matthew.d.roper@intel.com> >Cc: Gustavo Sousa <gustavo.sousa@intel.com> >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> >--- > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >index 81a96c52a92b..9c1007c44298 100644 >--- a/drivers/gpu/drm/i915/gt/intel_lrc.c >+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >@@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct intel_context *ce, u32 *cs) > cs, GEN12_GFX_CCS_AUX_NV); > > /* Wa_16014892111 */ >- if (IS_DG2(ce->engine->i915)) >+ if (IS_DG2(ce->engine->i915) || >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > cs = dg2_emit_draw_watermark_setting(cs); > > return cs; >-- >2.34.1 >
Hi Gustavo, > -----Original Message----- > From: Sousa, Gustavo <gustavo.sousa@intel.com> > Sent: Monday, May 15, 2023 7:45 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Sripada, Radhakrishna > <radhakrishna.sripada@intel.com>; Kalvala, Haridhar > <haridhar.kalvala@intel.com>; Roper, Matthew D > <matthew.d.roper@intel.com> > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A- > step > > Quoting Radhakrishna Sripada (2023-05-12 23:14:37) > >The dg2 workaround which is used for performance tuning > >is needed for Meteorlake A-step. > > > >v2: Limit the WA for A-step > > I think what Matt meant in the review for v1 was that this commit should > be rather about the tuning setting rather than the workaround itself. As > such, maybe we should change the commit message so that it focus on the > recommended tuning setting, i.e., instead of "Extend Wa_16014892111 to > MTL A-step" as subject, we should write something like "Apply > recommended tuning setting for ..." and give details. > > That said, since we are focusing on the tuning settings here, I guess > this could be squashed with the second patch and we could add a note > about DRAW_WATERMARK needing Wa_16014892111 for A steps of MTL. There are 2 aspects wrt. DRAW_WATERMARK. One that is a workaround which is applied on each context switch and is only applicable for DG2 and MTL-A step which is what this patch does. The other is the tuning parameter setting which is applicable for all of MTL which is a onetime configuration Handled by the next patch during ctx_workarounds_init. - Radhakrishna Sripada > > -- > Gustavo Sousa > > > > >Bspec: 68331 > >Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> > >Cc: Matt Roper <matthew.d.roper@intel.com> > >Cc: Gustavo Sousa <gustavo.sousa@intel.com> > >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > >--- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > b/drivers/gpu/drm/i915/gt/intel_lrc.c > >index 81a96c52a92b..9c1007c44298 100644 > >--- a/drivers/gpu/drm/i915/gt/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > >@@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct > intel_context *ce, u32 *cs) > > cs, GEN12_GFX_CCS_AUX_NV); > > > > /* Wa_16014892111 */ > >- if (IS_DG2(ce->engine->i915)) > >+ if (IS_DG2(ce->engine->i915) || > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > > cs = dg2_emit_draw_watermark_setting(cs); > > > > return cs; > >-- > >2.34.1 > >
On Mon, May 15, 2023 at 08:42:25AM -0700, Sripada, Radhakrishna wrote: > Hi Gustavo, > > > -----Original Message----- > > From: Sousa, Gustavo <gustavo.sousa@intel.com> > > Sent: Monday, May 15, 2023 7:45 AM > > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Sripada, Radhakrishna > > <radhakrishna.sripada@intel.com>; Kalvala, Haridhar > > <haridhar.kalvala@intel.com>; Roper, Matthew D > > <matthew.d.roper@intel.com> > > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A- > > step > > > > Quoting Radhakrishna Sripada (2023-05-12 23:14:37) > > >The dg2 workaround which is used for performance tuning > > >is needed for Meteorlake A-step. > > > > > >v2: Limit the WA for A-step > > > > I think what Matt meant in the review for v1 was that this commit should > > be rather about the tuning setting rather than the workaround itself. As > > such, maybe we should change the commit message so that it focus on the > > recommended tuning setting, i.e., instead of "Extend Wa_16014892111 to > > MTL A-step" as subject, we should write something like "Apply > > recommended tuning setting for ..." and give details. > > > > That said, since we are focusing on the tuning settings here, I guess > > this could be squashed with the second patch and we could add a note > > about DRAW_WATERMARK needing Wa_16014892111 for A steps of MTL. > > There are 2 aspects wrt. DRAW_WATERMARK. One that is a workaround > which is applied on each context switch and is only applicable for DG2 > and MTL-A step which is what this patch does. So just to be clear --- the workaround doesn't directly ask us to do anything specific with DRAW_WATERMARK. What the workaround says is that *if* we wind up needing to change the value of DRAW_WATERMARK away from the hardware default, then we need to handle the save/restore on each context switch ourselves since the hardware doesn't process this register properly on context switch and will lose its value. It turns out that MTL has a tuning setting that suggests changing DRAW_WATERMARK to a non-default value. Since the tuning setting is constant (i.e., it doesn't change at runtime based on other factors), we can ignore the "save" part of the workaround and just hardcode the "restore" part to the value specified by the tuning setting. But what we're programming here is still the tuning setting, it's just that the way we implement the tuning is adjusted by the workaround's guidance. It might make sense to swap the order of these patches --- make the first patch add the tuning setting (in the normal manner) for all steppings not impacted by the workaround. Then come back and apply the tuning setting in the special way on the A-step hardware to satisfy the guidance of Wa_16014892111. Or maybe it's simpler to just ignore the tuning setting on A-step entirely; that's a pre-production stepping of the platform, so it's not really going to get used for performance work anyway. If we don't bother programming the tuning on A-step, then we also don't need to worry about the workaround either. Matt > > The other is the tuning parameter setting which is applicable for all > of MTL which is a onetime configuration Handled by the next patch > during ctx_workarounds_init. > > - Radhakrishna Sripada > > > > > > -- > > Gustavo Sousa > > > > > > > >Bspec: 68331 > > >Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> > > >Cc: Matt Roper <matthew.d.roper@intel.com> > > >Cc: Gustavo Sousa <gustavo.sousa@intel.com> > > >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > >--- > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > >diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > > b/drivers/gpu/drm/i915/gt/intel_lrc.c > > >index 81a96c52a92b..9c1007c44298 100644 > > >--- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > >+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > >@@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct > > intel_context *ce, u32 *cs) > > > cs, GEN12_GFX_CCS_AUX_NV); > > > > > > /* Wa_16014892111 */ > > >- if (IS_DG2(ce->engine->i915)) > > >+ if (IS_DG2(ce->engine->i915) || > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > > > cs = dg2_emit_draw_watermark_setting(cs); > > > > > > return cs; > > >-- > > >2.34.1 > > >
Hi Matt, > -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Monday, May 15, 2023 10:28 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com> > Cc: Sousa, Gustavo <gustavo.sousa@intel.com>; intel- > gfx@lists.freedesktop.org; Justen, Jordan L <jordan.l.justen@intel.com>; > Kalvala, Haridhar <haridhar.kalvala@intel.com> > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A- > step > > On Mon, May 15, 2023 at 08:42:25AM -0700, Sripada, Radhakrishna wrote: > > Hi Gustavo, > > > > > -----Original Message----- > > > From: Sousa, Gustavo <gustavo.sousa@intel.com> > > > Sent: Monday, May 15, 2023 7:45 AM > > > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Sripada, Radhakrishna > > > <radhakrishna.sripada@intel.com>; Kalvala, Haridhar > > > <haridhar.kalvala@intel.com>; Roper, Matthew D > > > <matthew.d.roper@intel.com> > > > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL > A- > > > step > > > > > > Quoting Radhakrishna Sripada (2023-05-12 23:14:37) > > > >The dg2 workaround which is used for performance tuning > > > >is needed for Meteorlake A-step. > > > > > > > >v2: Limit the WA for A-step > > > > > > I think what Matt meant in the review for v1 was that this commit should > > > be rather about the tuning setting rather than the workaround itself. As > > > such, maybe we should change the commit message so that it focus on the > > > recommended tuning setting, i.e., instead of "Extend Wa_16014892111 to > > > MTL A-step" as subject, we should write something like "Apply > > > recommended tuning setting for ..." and give details. > > > > > > That said, since we are focusing on the tuning settings here, I guess > > > this could be squashed with the second patch and we could add a note > > > about DRAW_WATERMARK needing Wa_16014892111 for A steps of MTL. > > > > There are 2 aspects wrt. DRAW_WATERMARK. One that is a workaround > > which is applied on each context switch and is only applicable for DG2 > > and MTL-A step which is what this patch does. > > So just to be clear --- the workaround doesn't directly ask us to do > anything specific with DRAW_WATERMARK. What the workaround says is that > *if* we wind up needing to change the value of DRAW_WATERMARK away from > the hardware default, then we need to handle the save/restore on each > context switch ourselves since the hardware doesn't process this > register properly on context switch and will lose its value. > > It turns out that MTL has a tuning setting that suggests changing > DRAW_WATERMARK to a non-default value. Since the tuning setting is > constant (i.e., it doesn't change at runtime based on other factors), we > can ignore the "save" part of the workaround and just hardcode the > "restore" part to the value specified by the tuning setting. But what > we're programming here is still the tuning setting, it's just that the > way we implement the tuning is adjusted by the workaround's guidance. > > It might make sense to swap the order of these patches --- make the > first patch add the tuning setting (in the normal manner) for all > steppings not impacted by the workaround. Then come back and apply the > tuning setting in the special way on the A-step hardware to satisfy the > guidance of Wa_16014892111. Or maybe it's simpler to just ignore the > tuning setting on A-step entirely; that's a pre-production stepping of > the platform, so it's not really going to get used for performance work > anyway. If we don't bother programming the tuning on A-step, then we > also don't need to worry about the workaround either. Thank you for the explanation. I was inclined to drop the WA but we do have B-step parts in CI which have A-step GT hence trying to keep this around. I resend the patches swapping the order and adding better explanation. - Radhakrishna Sripada > > > Matt > > > > > The other is the tuning parameter setting which is applicable for all > > of MTL which is a onetime configuration Handled by the next patch > > during ctx_workarounds_init. > > > > - Radhakrishna Sripada > > > > > > > > > > -- > > > Gustavo Sousa > > > > > > > > > > >Bspec: 68331 > > > >Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> > > > >Cc: Matt Roper <matthew.d.roper@intel.com> > > > >Cc: Gustavo Sousa <gustavo.sousa@intel.com> > > > >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > >--- > > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > >diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >index 81a96c52a92b..9c1007c44298 100644 > > > >--- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >@@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct > > > intel_context *ce, u32 *cs) > > > > cs, GEN12_GFX_CCS_AUX_NV); > > > > > > > > /* Wa_16014892111 */ > > > >- if (IS_DG2(ce->engine->i915)) > > > >+ if (IS_DG2(ce->engine->i915) || > > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) > || > > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > > > > cs = dg2_emit_draw_watermark_setting(cs); > > > > > > > > return cs; > > > >-- > > > >2.34.1 > > > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 81a96c52a92b..9c1007c44298 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct intel_context *ce, u32 *cs) cs, GEN12_GFX_CCS_AUX_NV); /* Wa_16014892111 */ - if (IS_DG2(ce->engine->i915)) + if (IS_DG2(ce->engine->i915) || + IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || + IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) cs = dg2_emit_draw_watermark_setting(cs); return cs;
The dg2 workaround which is used for performance tuning is needed for Meteorlake A-step. v2: Limit the WA for A-step Bspec: 68331 Cc: Haridhar Kalvala <haridhar.kalvala@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Gustavo Sousa <gustavo.sousa@intel.com> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)