On Tue, Dec 10, 2024 at 01:52:21PM +0100, Andi Shyti wrote:
> Hi Raag,
>
> > +/* Wa_14022698537:dg2 */
> > +static void i915_enable_g8(struct drm_i915_private *i915)
> > +{
> > + if (IS_DG2(i915)) {
> > + if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> > + return;
> > +
> > + snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> > + POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> > + }
>
> In the workaround description there an "else if" which I am not
> understanding. I it suggesting to do nothing or is it suggesting
> to do the same thing?
We do the same thing (apply WA) except when the _D IDs are not paired
with whitelisted CPUs. What I did here is added a return call with
inverted CPU logic and got rid of the else part. I know it looks quirky
but does the job.
> > +}
> > +
> > static int i915_pcode_init(struct drm_i915_private *i915)
> > {
> > struct intel_gt *gt;
> > @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
> > }
> > }
> >
> > + i915_enable_g8(i915);
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 89e4381f8baa..d400c77423a5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3617,6 +3617,7 @@
> > #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)
> > +#define POWER_SETUP_SUBCOMMAND_G8_ENABLE 0x6
>
> for aesthetics 0x06 would look better, but this should be
> changed everywhere in the file because single digit hex values
> are used. It's better to keep uniformity in the style.
I agree about uniformity but perhaps a separate filewide patch would
be much cleaner IMHO. Let's keep it as it is for now.
> > #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US 0x23
> > #define XEHP_PCODE_FREQUENCY_CONFIG 0x6e /* pvc */
> > /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> > --
> > 2.34.1
