| Message ID | 20241011103250.1035316-5-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Wa_14022698537 | expand |
Hi Raag On 10/11/2024 4:02 PM, Raag Jadav wrote: > G8 power state entry is disabled due to a limitation on DG2, so we > enable it from driver with Wa_14022698537. Fow now we enable it for typo > all DG2 devices with the exception of a few, for which, we enable > only when paired with whitelisted CPU models. > > v2: Fix Wa_ID and include it in subject (Badal) > Rephrase commit message (Jani) > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index e539a656cfc3..bcd7630c1631 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -14,6 +14,7 @@ > #include "intel_gt_mcr.h" > #include "intel_gt_print.h" > #include "intel_gt_regs.h" > +#include "intel_pcode.h" > #include "intel_ring.h" > #include "intel_workarounds.h" > > @@ -1770,9 +1771,26 @@ static void wa_list_apply(const struct i915_wa_list *wal) > intel_gt_mcr_unlock(gt, flags); > } > > +/* Wa_14022698537:dg2 */ > +static void intel_enable_g8(struct intel_uncore *uncore) > +{ > + struct drm_i915_private *i915 = uncore->i915; > + > + if (IS_DG2(i915)) { > + if (IS_DG2_WA(i915) && !intel_match_wa_cpu()) > + return; > + > + snb_pcode_write_p(uncore, PCODE_POWER_SETUP, > + POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0); > + } > +} > + > void intel_gt_apply_workarounds(struct intel_gt *gt) > { > wa_list_apply(>->wa_list); > + > + /* Special case for pcode mailbox which can't be on wa_list */ > + intel_enable_g8(gt->uncore); > } > > static bool wa_list_verify(struct intel_gt *gt, > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 41f4350a7c6c..e948b194550c 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3568,6 +3568,7 @@ > #define PCODE_POWER_SETUP 0x7C > #define POWER_SETUP_SUBCOMMAND_READ_I1 0x4 > #define POWER_SETUP_SUBCOMMAND_WRITE_I1 0x5 > +#define POWER_SETUP_SUBCOMMAND_G8_ENABLE 0x6 Add this below the I1 bits Thanks, Riana > #define POWER_SETUP_I1_WATTS REG_BIT(31) > #define POWER_SETUP_I1_SHIFT 6 /* 10.6 fixed point format */ > #define POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
On 11-10-2024 16:02, Raag Jadav wrote: > G8 power state entry is disabled due to a limitation on DG2, so we > enable it from driver with Wa_14022698537. Fow now we enable it for > all DG2 devices with the exception of a few, for which, we enable > only when paired with whitelisted CPU models. > > v2: Fix Wa_ID and include it in subject (Badal) > Rephrase commit message (Jani) > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index e539a656cfc3..bcd7630c1631 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -14,6 +14,7 @@ > #include "intel_gt_mcr.h" > #include "intel_gt_print.h" > #include "intel_gt_regs.h" > +#include "intel_pcode.h" > #include "intel_ring.h" > #include "intel_workarounds.h" > > @@ -1770,9 +1771,26 @@ static void wa_list_apply(const struct i915_wa_list *wal) > intel_gt_mcr_unlock(gt, flags); > } > > +/* Wa_14022698537:dg2 */ > +static void intel_enable_g8(struct intel_uncore *uncore) > +{ > + struct drm_i915_private *i915 = uncore->i915; > + > + if (IS_DG2(i915)) { > + if (IS_DG2_WA(i915) && !intel_match_wa_cpu()) > + return; > + > + snb_pcode_write_p(uncore, PCODE_POWER_SETUP, > + POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0); > + } > +} > + > void intel_gt_apply_workarounds(struct intel_gt *gt) > { > wa_list_apply(>->wa_list); > + > + /* Special case for pcode mailbox which can't be on wa_list */ > + intel_enable_g8(gt->uncore); This workaround is not specific to GT; G8 is a state specific to the SoC. Therefore, move this workaround above the GT layer and pass argument i915->uncore instead of gt->uncore Regards, Badal > } > > static bool wa_list_verify(struct intel_gt *gt, > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 41f4350a7c6c..e948b194550c 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3568,6 +3568,7 @@ > #define PCODE_POWER_SETUP 0x7C > #define POWER_SETUP_SUBCOMMAND_READ_I1 0x4 > #define POWER_SETUP_SUBCOMMAND_WRITE_I1 0x5 > +#define POWER_SETUP_SUBCOMMAND_G8_ENABLE 0x6 > #define POWER_SETUP_I1_WATTS REG_BIT(31) > #define POWER_SETUP_I1_SHIFT 6 /* 10.6 fixed point format */ > #define POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
On Tue, Oct 22, 2024 at 06:41:57PM +0530, Nilawar, Badal wrote: > On 11-10-2024 16:02, Raag Jadav wrote: > > G8 power state entry is disabled due to a limitation on DG2, so we > > enable it from driver with Wa_14022698537. Fow now we enable it for > > all DG2 devices with the exception of a few, for which, we enable > > only when paired with whitelisted CPU models. > > > > v2: Fix Wa_ID and include it in subject (Badal) > > Rephrase commit message (Jani) > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 ++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > index e539a656cfc3..bcd7630c1631 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > @@ -14,6 +14,7 @@ > > #include "intel_gt_mcr.h" > > #include "intel_gt_print.h" > > #include "intel_gt_regs.h" > > +#include "intel_pcode.h" > > #include "intel_ring.h" > > #include "intel_workarounds.h" > > @@ -1770,9 +1771,26 @@ static void wa_list_apply(const struct i915_wa_list *wal) > > intel_gt_mcr_unlock(gt, flags); > > } > > +/* Wa_14022698537:dg2 */ > > +static void intel_enable_g8(struct intel_uncore *uncore) > > +{ > > + struct drm_i915_private *i915 = uncore->i915; > > + > > + if (IS_DG2(i915)) { > > + if (IS_DG2_WA(i915) && !intel_match_wa_cpu()) > > + return; > > + > > + snb_pcode_write_p(uncore, PCODE_POWER_SETUP, > > + POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0); > > + } > > +} > > + > > void intel_gt_apply_workarounds(struct intel_gt *gt) > > { > > wa_list_apply(>->wa_list); > > + > > + /* Special case for pcode mailbox which can't be on wa_list */ > > + intel_enable_g8(gt->uncore); > > This workaround is not specific to GT; G8 is a state specific to the SoC. > Therefore, move this workaround above the GT layer and pass argument > i915->uncore instead of gt->uncore Since this WA needs to be applied on suspend/resume/reset cycles, I found it to be more suitable here, atleast according to the documentation. * - GT workarounds: the list of these WAs is applied whenever these registers * revert to their default values: on GPU reset, suspend/resume [1]_, etc. We can either limit this to primary gt (using gt->info.id) here or move this to i915_pcode_init() instead, whichever is the better option. Raag
> -----Original Message----- > From: Jadav, Raag <raag.jadav@intel.com> > Sent: Wednesday, October 23, 2024 12:40 PM > To: Nilawar, Badal <badal.nilawar@intel.com> > Cc: jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com; Vivi, Rodrigo > <rodrigo.vivi@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; > andi.shyti@linux.intel.com; intel-gfx@lists.freedesktop.org; Gupta, Anshuman > <anshuman.gupta@intel.com>; Tauro, Riana <riana.tauro@intel.com> > Subject: Re: [PATCH v2 4/4] drm/i915/dg2: Implement Wa_14022698537 > > On Tue, Oct 22, 2024 at 06:41:57PM +0530, Nilawar, Badal wrote: > > On 11-10-2024 16:02, Raag Jadav wrote: > > > G8 power state entry is disabled due to a limitation on DG2, so we > > > enable it from driver with Wa_14022698537. Fow now we enable it for > > > all DG2 devices with the exception of a few, for which, we enable > > > only when paired with whitelisted CPU models. > > > > > > v2: Fix Wa_ID and include it in subject (Badal) > > > Rephrase commit message (Jani) > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 ++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > > 2 files changed, 19 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > index e539a656cfc3..bcd7630c1631 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > @@ -14,6 +14,7 @@ > > > #include "intel_gt_mcr.h" > > > #include "intel_gt_print.h" > > > #include "intel_gt_regs.h" > > > +#include "intel_pcode.h" > > > #include "intel_ring.h" > > > #include "intel_workarounds.h" > > > @@ -1770,9 +1771,26 @@ static void wa_list_apply(const struct > i915_wa_list *wal) > > > intel_gt_mcr_unlock(gt, flags); > > > } > > > +/* Wa_14022698537:dg2 */ > > > +static void intel_enable_g8(struct intel_uncore *uncore) { > > > + struct drm_i915_private *i915 = uncore->i915; > > > + > > > + if (IS_DG2(i915)) { > > > + if (IS_DG2_WA(i915) && !intel_match_wa_cpu()) > > > + return; > > > + > > > + snb_pcode_write_p(uncore, PCODE_POWER_SETUP, > > > + POWER_SETUP_SUBCOMMAND_G8_ENABLE, > 0, 0); > > > + } > > > +} > > > + > > > void intel_gt_apply_workarounds(struct intel_gt *gt) > > > { > > > wa_list_apply(>->wa_list); > > > + > > > + /* Special case for pcode mailbox which can't be on wa_list */ > > > + intel_enable_g8(gt->uncore); My earlier comment was to include this WA in intel_gt_apply_workarounds with an intention to Add to wa_list_apply() but seems it is not feasible to keep in that list. Therefore it is better to keep call this WA function from i915_pcode_init to wrap all pcode related Functionality. Thanks, Anshuman Gupta. > > > > This workaround is not specific to GT; G8 is a state specific to the SoC. > > Therefore, move this workaround above the GT layer and pass argument > > i915->uncore instead of gt->uncore > > Since this WA needs to be applied on suspend/resume/reset cycles, I found it to > be more suitable here, atleast according to the documentation. > > * - GT workarounds: the list of these WAs is applied whenever these registers > * revert to their default values: on GPU reset, suspend/resume [1]_, etc. > > We can either limit this to primary gt (using gt->info.id) here or move this to > i915_pcode_init() instead, whichever is the better option. > > Raag
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index e539a656cfc3..bcd7630c1631 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -14,6 +14,7 @@ #include "intel_gt_mcr.h" #include "intel_gt_print.h" #include "intel_gt_regs.h" +#include "intel_pcode.h" #include "intel_ring.h" #include "intel_workarounds.h" @@ -1770,9 +1771,26 @@ static void wa_list_apply(const struct i915_wa_list *wal) intel_gt_mcr_unlock(gt, flags); } +/* Wa_14022698537:dg2 */ +static void intel_enable_g8(struct intel_uncore *uncore) +{ + struct drm_i915_private *i915 = uncore->i915; + + if (IS_DG2(i915)) { + if (IS_DG2_WA(i915) && !intel_match_wa_cpu()) + return; + + snb_pcode_write_p(uncore, PCODE_POWER_SETUP, + POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0); + } +} + void intel_gt_apply_workarounds(struct intel_gt *gt) { wa_list_apply(>->wa_list); + + /* Special case for pcode mailbox which can't be on wa_list */ + intel_enable_g8(gt->uncore); } static bool wa_list_verify(struct intel_gt *gt, diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 41f4350a7c6c..e948b194550c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3568,6 +3568,7 @@ #define PCODE_POWER_SETUP 0x7C #define POWER_SETUP_SUBCOMMAND_READ_I1 0x4 #define POWER_SETUP_SUBCOMMAND_WRITE_I1 0x5 +#define POWER_SETUP_SUBCOMMAND_G8_ENABLE 0x6 #define POWER_SETUP_I1_WATTS REG_BIT(31) #define POWER_SETUP_I1_SHIFT 6 /* 10.6 fixed point format */ #define POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
G8 power state entry is disabled due to a limitation on DG2, so we enable it from driver with Wa_14022698537. Fow now we enable it for all DG2 devices with the exception of a few, for which, we enable only when paired with whitelisted CPU models. v2: Fix Wa_ID and include it in subject (Badal) Rephrase commit message (Jani) Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 19 insertions(+)