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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> 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.

Well, we don't use that for individual workarounds or hacks either. When
you think of using something like that, see what git grep says.

> Any better way?

You probably need to turn it into another subplatform, and add it in
intel_device_info.c. You're probably going to need to rehash the
INTEL_DG2_*_IDS PCI ID macros too. That's how we tell platforms apart at
the PCI ID granularity.

BR,
Jani.


>
> Raag

-- 
Jani Nikula, Intel