| Message ID | 20230418220446.2205509-4-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More MTL WA and powerwell patches | expand |
On Tue, Apr 18, 2023 at 03:04:45PM -0700, Radhakrishna Sripada wrote: > From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > > Wa_22011802037 was being applied to all graphics_ver 11 & 12. This patch > updates the if statement to apply the W/A to right platforms and extends > it to MTL-M:A step. > Bspec: 53509 > v1.1: Fix checkpatch warning. > v2: Change the check to reflect the wa at other palces(Lucas) s/palces/places. > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> With that. Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com> > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index 88e881b100cf..ee3e8352637f 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct intel_context *ce, u32 head, bool scrub) > > static void guc_engine_reset_prepare(struct intel_engine_cs *engine) > { > - if (!IS_GRAPHICS_VER(engine->i915, 11, 12)) > - return; > - > - intel_engine_stop_cs(engine); > - > /* > * Wa_22011802037: In addition to stopping the cs, we need > * to wait for any pending mi force wakeups > */ > - intel_engine_wait_for_pending_mi_fw(engine); > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > + (GRAPHICS_VER(engine->i915) >= 11 && > + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) { > + intel_engine_stop_cs(engine); > + intel_engine_wait_for_pending_mi_fw(engine); > + } > } > > static void guc_reset_nop(struct intel_engine_cs *engine) > -- > 2.34.1 >
On Wed, Apr 19, 2023 at 02:40:33PM -0700, Matt Atwood wrote: > On Tue, Apr 18, 2023 at 03:04:45PM -0700, Radhakrishna Sripada wrote: > > From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > > > > Wa_22011802037 was being applied to all graphics_ver 11 & 12. This patch > > updates the if statement to apply the W/A to right platforms and extends > > it to MTL-M:A step. > > > Bspec: 53509 > > v1.1: Fix checkpatch warning. > > v2: Change the check to reflect the wa at other palces(Lucas) > s/palces/places. > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > With that. > Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com> > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> It doesn't look like this patch is complete? It's only changing one condition for Wa_22011802037, even though there are several in the code. From a quick grep, you're still missing updates for at least guc_ctl_wa_flags, execlists_reset_prepare, and __intel_engine_stop_cs. Since this workaround is a complicated one that touches so many areas of the code, and has a complex platform list, it's probably time to factor the condition out into a needs_wa_22011802037() helper or something. Matt > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index 88e881b100cf..ee3e8352637f 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct intel_context *ce, u32 head, bool scrub) > > > > static void guc_engine_reset_prepare(struct intel_engine_cs *engine) > > { > > - if (!IS_GRAPHICS_VER(engine->i915, 11, 12)) > > - return; > > - > > - intel_engine_stop_cs(engine); > > - > > /* > > * Wa_22011802037: In addition to stopping the cs, we need > > * to wait for any pending mi force wakeups > > */ > > - intel_engine_wait_for_pending_mi_fw(engine); > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > > + (GRAPHICS_VER(engine->i915) >= 11 && > > + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) { > > + intel_engine_stop_cs(engine); > > + intel_engine_wait_for_pending_mi_fw(engine); > > + } > > } > > > > static void guc_reset_nop(struct intel_engine_cs *engine) > > -- > > 2.34.1 > >
On Fri, Apr 21, 2023 at 08:05:50AM -0700, Matt Roper wrote: > On Wed, Apr 19, 2023 at 02:40:33PM -0700, Matt Atwood wrote: > > On Tue, Apr 18, 2023 at 03:04:45PM -0700, Radhakrishna Sripada wrote: > > > From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > > > > > > Wa_22011802037 was being applied to all graphics_ver 11 & 12. This patch > > > updates the if statement to apply the W/A to right platforms and extends > > > it to MTL-M:A step. > > > > > Bspec: 53509 > > > v1.1: Fix checkpatch warning. > > > v2: Change the check to reflect the wa at other palces(Lucas) > > s/palces/places. > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > With that. > > Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com> > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > It doesn't look like this patch is complete? It's only changing one > condition for Wa_22011802037, even though there are several in the code. > From a quick grep, you're still missing updates for at least > guc_ctl_wa_flags, execlists_reset_prepare, and __intel_engine_stop_cs. Actually, scratch that. Those other spots already have a MTL clause as part of the condition. But in that case it means the commit message here is inaccurate; you're not extending this workaround to MTL a-step because that already happened on a previous patch. You're just providing a fix for an incomplete implementation that happened earlier. The commit message should be explaining that. > > Since this workaround is a complicated one that touches so many areas of > the code, and has a complex platform list, it's probably time to factor > the condition out into a needs_wa_22011802037() helper or something. I still suggest doing this, especially since we've clearly screwed up the handling of this workaround at least once already. Matt > > > Matt > > > > --- > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > index 88e881b100cf..ee3e8352637f 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct intel_context *ce, u32 head, bool scrub) > > > > > > static void guc_engine_reset_prepare(struct intel_engine_cs *engine) > > > { > > > - if (!IS_GRAPHICS_VER(engine->i915, 11, 12)) > > > - return; > > > - > > > - intel_engine_stop_cs(engine); > > > - > > > /* > > > * Wa_22011802037: In addition to stopping the cs, we need > > > * to wait for any pending mi force wakeups > > > */ > > > - intel_engine_wait_for_pending_mi_fw(engine); > > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > > > + (GRAPHICS_VER(engine->i915) >= 11 && > > > + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) { > > > + intel_engine_stop_cs(engine); > > > + intel_engine_wait_for_pending_mi_fw(engine); > > > + } > > > } > > > > > > static void guc_reset_nop(struct intel_engine_cs *engine) > > > -- > > > 2.34.1 > > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Friday, April 21, 2023 8:09 AM > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > Cc: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2 3/4] drm/i915/mtl: Extend Wa_22011802037 > to MTL A-step > > On Fri, Apr 21, 2023 at 08:05:50AM -0700, Matt Roper wrote: > > On Wed, Apr 19, 2023 at 02:40:33PM -0700, Matt Atwood wrote: > > > On Tue, Apr 18, 2023 at 03:04:45PM -0700, Radhakrishna Sripada wrote: > > > > From: Madhumitha Tolakanahalli Pradeep > <madhumitha.tolakanahalli.pradeep@intel.com> > > > > > > > > Wa_22011802037 was being applied to all graphics_ver 11 & 12. This patch > > > > updates the if statement to apply the W/A to right platforms and extends > > > > it to MTL-M:A step. > > > > > > > Bspec: 53509 > > > > v1.1: Fix checkpatch warning. > > > > v2: Change the check to reflect the wa at other palces(Lucas) > > > s/palces/places. > > > > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > > With that. > > > Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep > <madhumitha.tolakanahalli.pradeep@intel.com> > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > > It doesn't look like this patch is complete? It's only changing one > > condition for Wa_22011802037, even though there are several in the code. > > From a quick grep, you're still missing updates for at least > > guc_ctl_wa_flags, execlists_reset_prepare, and __intel_engine_stop_cs. > > Actually, scratch that. Those other spots already have a MTL clause as > part of the condition. But in that case it means the commit message > here is inaccurate; you're not extending this workaround to MTL a-step > because that already happened on a previous patch. You're just > providing a fix for an incomplete implementation that happened earlier. > The commit message should be explaining that. > > > > > Since this workaround is a complicated one that touches so many areas of > > the code, and has a complex platform list, it's probably time to factor > > the condition out into a needs_wa_22011802037() helper or something. > > I still suggest doing this, especially since we've clearly screwed up > the handling of this workaround at least once already. > Sure Matt. With the patch already merged, I will handle this as a separate patch. - Radhakrishna(RK) Sripada > > Matt > > > > > > > Matt > > > > > > --- > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > index 88e881b100cf..ee3e8352637f 100644 > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct > intel_context *ce, u32 head, bool scrub) > > > > > > > > static void guc_engine_reset_prepare(struct intel_engine_cs *engine) > > > > { > > > > - if (!IS_GRAPHICS_VER(engine->i915, 11, 12)) > > > > - return; > > > > - > > > > - intel_engine_stop_cs(engine); > > > > - > > > > /* > > > > * Wa_22011802037: In addition to stopping the cs, we need > > > > * to wait for any pending mi force wakeups > > > > */ > > > > - intel_engine_wait_for_pending_mi_fw(engine); > > > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || > > > > + (GRAPHICS_VER(engine->i915) >= 11 && > > > > + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) { > > > > + intel_engine_stop_cs(engine); > > > > + intel_engine_wait_for_pending_mi_fw(engine); > > > > + } > > > > } > > > > > > > > static void guc_reset_nop(struct intel_engine_cs *engine) > > > > -- > > > > 2.34.1 > > > > > > > > -- > > Matt Roper > > Graphics Software Engineer > > Linux GPU Platform Enablement > > Intel Corporation > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 88e881b100cf..ee3e8352637f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct intel_context *ce, u32 head, bool scrub) static void guc_engine_reset_prepare(struct intel_engine_cs *engine) { - if (!IS_GRAPHICS_VER(engine->i915, 11, 12)) - return; - - intel_engine_stop_cs(engine); - /* * Wa_22011802037: In addition to stopping the cs, we need * to wait for any pending mi force wakeups */ - intel_engine_wait_for_pending_mi_fw(engine); + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) || + (GRAPHICS_VER(engine->i915) >= 11 && + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) { + intel_engine_stop_cs(engine); + intel_engine_wait_for_pending_mi_fw(engine); + } } static void guc_reset_nop(struct intel_engine_cs *engine)