[PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Andi Shyti]

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 at linux.intel.com>

> > > +}
> > > +
> > >  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