diff mbox series

[v2,4/4] drm/i915/dg2: Implement Wa_14022698537

Message ID 20241011103250.1035316-5-raag.jadav@intel.com (mailing list archive)
State New
Headers show
Series Implement Wa_14022698537 | expand

Commit Message

Raag Jadav Oct. 11, 2024, 10:32 a.m. UTC
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(+)

Comments

Riana Tauro Oct. 15, 2024, 10:02 a.m. UTC | #1
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(&gt->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)
Nilawar, Badal Oct. 22, 2024, 1:11 p.m. UTC | #2
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(&gt->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)
Raag Jadav Oct. 23, 2024, 7:09 a.m. UTC | #3
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(&gt->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
Gupta, Anshuman Oct. 23, 2024, 8:25 a.m. UTC | #4
> -----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(&gt->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 mbox series

Patch

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(&gt->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)