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

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
> > 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@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