[PATCH v1] drm/i915/dg2: enable G8 with a workaround

[Gupta, Anshuman]

> -----Original Message-----
> From: Jadav, Raag <raag.jadav at intel.com>
> Sent: Wednesday, October 9, 2024 6:18 PM
> To: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: joonas.lahtinen at linux.intel.com; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> Roper, Matthew D <matthew.d.roper at intel.com>; andi.shyti at linux.intel.com;
> intel-gfx at lists.freedesktop.org; Gupta, Anshuman
> <anshuman.gupta at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> Tauro, Riana <riana.tauro at intel.com>
> 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 at intel.com> 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 at intel.com>
> > > ---
> > >  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