On Wed, Dec 11, 2024 at 07:21:54AM +0200, Raag Jadav wrote:
> 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.
Thanks... the document is weirdly fromatted and I wanted to make
sure everything is aligned.
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> > > +}
> > > +
> > > 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.
Yes, that was my point. The problem with a separate patch is that
you would screw up git blame because you would touch so many
lines with a trivial change. Therefore, it's better to leave it
as it is.
Thanks,
Andi
> > > #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
