Re: [PATCH v2 3/4] drm/i915/wa: Introduce intel_wa_cpu.c for CPU specific workarounds

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

On Fri, 11 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> Having similar naming convention in intel-family.h and intel_device_info.h
> results in redefinition of a few platforms. Define CPU IDs in its own file
> to avoid this.
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile               |  1 +
>  drivers/gpu/drm/i915/gt/intel_wa_cpu.c      | 34 +++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_workarounds.h |  2 ++
>  3 files changed, 37 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/gt/intel_wa_cpu.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index c63fa2133ccb..1f9b503ab976 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -121,6 +121,7 @@ gt-y += \
>  	gt/intel_timeline.o \
>  	gt/intel_tlb.o \
>  	gt/intel_wopcm.o \
> +	gt/intel_wa_cpu.o \
>  	gt/intel_workarounds.o \
>  	gt/shmem_utils.o \
>  	gt/sysfs_engines.o
> diff --git a/drivers/gpu/drm/i915/gt/intel_wa_cpu.c b/drivers/gpu/drm/i915/gt/intel_wa_cpu.c
> new file mode 100644
> index 000000000000..cbdab13e9db6
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_wa_cpu.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + *
> + * This file is introduced to avoid platform redefinition from
> + * intel_device_info.h :(

I think the comment is good to have, but please be more specific and
direct, and leave out the emoticons.

"Avoid INTEL_<PLATFORM> name collisions between asm/intel-family.h and
intel_device_info.h by having a separate file."

Or something like that. Spells out exactly what the problem is, instead
of leaving the reader guessing.

> + */
> +
> +#include "intel_workarounds.h"
> +
> +#ifdef CONFIG_X86
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static const struct x86_cpu_id wa_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),
> +	{}
> +};
> +
> +bool intel_match_wa_cpu(void)

IMO the name's too generic.

> +{
> +	return x86_match_cpu(wa_cpu_ids);
> +}
> +#else
> +bool intel_match_wa_cpu(void) { return false; }
> +#endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.h b/drivers/gpu/drm/i915/gt/intel_workarounds.h
> index 9beaab77c7f0..12f24fb31363 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.h
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.h
> @@ -21,6 +21,8 @@ static inline void intel_wa_list_free(struct i915_wa_list *wal)
>  	memset(wal, 0, sizeof(*wal));
>  }
>  
> +bool intel_match_wa_cpu(void);
> +
>  void intel_engine_init_ctx_wa(struct intel_engine_cs *engine);
>  int intel_engine_emit_ctx_wa(struct i915_request *rq);

-- 
Jani Nikula, Intel