> -----Original Message----- > From: Jadav, Raag <raag.jadav@xxxxxxxxx> > Sent: Wednesday, October 9, 2024 6:18 PM > To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Cc: joonas.lahtinen@xxxxxxxxxxxxxxx; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; > Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; andi.shyti@xxxxxxxxxxxxxxx; > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Gupta, Anshuman > <anshuman.gupta@xxxxxxxxx>; Nilawar, Badal <badal.nilawar@xxxxxxxxx>; > Tauro, Riana <riana.tauro@xxxxxxxxx> > Subject: Re: [PATCH v1] drm/i915/dg2: enable G8 with a workaround > > On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote: > > On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@xxxxxxxxx> wrote: > > > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we AFAIU it discrete card firmware not HOST bios. Thanks, Anshuman. > > > enable it from kernel with Wa_14022698589. Currently it is enabled > > > for all DG2 devices with the exception of a few, for which, it is > > > enabled only when paired with whitelisted CPU models. > > > > In commit messages "currently" and the present tense usually refer to > > the status quo before the patch has been merged. Doesn't seem to be > > the case here, and it confuses what we have now and what the patch changes. > > True. > > > > > > > Signed-off-by: Raag Jadav <raag.jadav@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 > +++++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > index e539a656cfc3..b2db51377488 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > @@ -14,11 +14,30 @@ > > > #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" > > > > > > #include "display/intel_fbc_regs.h" > > > > > > +#ifdef CONFIG_X86 > > > +#include <asm/cpu_device_id.h> > > > +#include <asm/intel-family.h> > > > + > > > +static const struct x86_cpu_id g8_cpu_ids[] = { > > > + X86_MATCH_VFM(INTEL_ALDERLAKE, NULL), > > > + X86_MATCH_VFM(INTEL_ALDERLAKE_L, NULL), > > > + X86_MATCH_VFM(INTEL_COMETLAKE, NULL), > > > + X86_MATCH_VFM(INTEL_KABYLAKE, NULL), > > > + X86_MATCH_VFM(INTEL_KABYLAKE_L, NULL), > > > + X86_MATCH_VFM(INTEL_RAPTORLAKE, NULL), > > > + X86_MATCH_VFM(INTEL_RAPTORLAKE_P, NULL), > > > + X86_MATCH_VFM(INTEL_RAPTORLAKE_S, NULL), > > > + X86_MATCH_VFM(INTEL_ROCKETLAKE, NULL), > > > + {} > > > +}; > > > +#endif > > > + > > > /** > > > * DOC: Hardware workarounds > > > * > > > @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct > i915_wa_list *wal) > > > intel_gt_mcr_unlock(gt, flags); > > > } > > > > > > +#define DG2_G8_WA_RANGE_1 0x56A0 ... 0x56AF > > > +#define DG2_G8_WA_RANGE_2 0x56B0 ... 0x56B9 > > > > Absolutely not. > > I had an "ugly" self-note which I forgot to add while sending out :D > > > > + > > > +/* Wa_14022698589:dg2 */ > > > +static void intel_enable_g8(struct intel_uncore *uncore) { > > > + if (IS_DG2(uncore->i915)) { > > > + switch (INTEL_DEVID(uncore->i915)) { > > > > Even using INTEL_DEVID() is a no-go. There are currently four users, > > and even some of them are too much. > > > > We try hard to abstract this stuff at a higher level, and there must > > be zero direct PCI ID checks in code other than the table driven > > device identification. Otherwise it's just impossible to figure out > > where we do platform specific stuff for each platform. > > Even if we use pci_match_id(), we'd need an explicit list to match against. > Any better way? > > Raag