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