[Intel-xe] [PATCH v4 10/12] drm/xe: Add missing includes for i915_reg_defs.h and others

Sat Feb 25 05:33:57 UTC 2023

On Fri, Feb 24, 2023 at 04:48:36PM -0800, Matt Roper wrote:
>On Fri, Feb 24, 2023 at 04:15:46PM -0800, Lucas De Marchi wrote:
>> Several places were missing the i915_reg_defs.h include. Any place using
>> the i915_reg_t typedef or macros like _MMIO(), FIELD_PREP(), etc should
>> include the register defining them explicitly.
>>
>> This has gone unnoticed due to the way the display is integrated. By
>> including the display headers the i915_reg_defs.h is indirectly
>> included. Also, although the Makefile tries to add more cflags for the
>> display part, it fails in doing so: those cflags are actually added to
>> the entire Makefile (see subdir-ccflags documentation).
>>
>> This change is obtained by temporarily removing display from xe and
>> fixing the breakages.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c       | 6 ++++--
>>  drivers/gpu/drm/xe/xe_gt_pagefault.c | 2 ++
>>  drivers/gpu/drm/xe/xe_gt_topology.c  | 2 ++
>>  drivers/gpu/drm/xe/xe_migrate.c      | 2 ++
>>  drivers/gpu/drm/xe/xe_pcode_api.h    | 2 ++
>>  drivers/gpu/drm/xe/xe_step.c         | 2 ++
>>  6 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 45e3c740fda2..8f2b2acd4024 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -31,6 +31,10 @@
>>  #include "xe_vm_madvise.h"
>>  #include "xe_wait_user_fence.h"
>>
>> +#include "../i915/i915_reg_defs.h"
>> +
>> +#define SOFTWARE_FLAGS_SPR33         _MMIO(0x4F084)
>> +
>>  static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>>  {
>>  	struct xe_file *xef;
>> @@ -405,8 +409,6 @@ static void device_kill_persitent_engines(struct xe_device *xe,
>>  	mutex_unlock(&xe->persitent_engines.lock);
>>  }
>>
>> -#define SOFTWARE_FLAGS_SPR33         _MMIO(0x4F084)
>> -
>>  void xe_device_wmb(struct xe_device *xe)
>>  {
>>  	struct xe_gt *gt = xe_device_get_gt(xe, 0);
>> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> index 0e7047b89a83..19403dbea9f5 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> @@ -20,6 +20,8 @@
>>  #include "xe_trace.h"
>>  #include "xe_vm.h"
>>
>> +#include "../i915/i915_reg_defs.h"
>> +
>
>Does this file actually use any i915/xe register stuff or is it just the
>generic FIELD_GET() definitions causing a problem?  If the latter, then
>maybe
>
>        #include <linux/bitfield.h>
>
>would be more appropriate?

yep, looks like we can do with only linux/bitfield.h

Main issue in some place is the use of REG_BIT, REG_GENMASK,
_MMIO and MCR_REG, that are i915ism. But here none of those are used.

For REG_BIT/REG_GENMASK, I think what we want longterm is to add a
REG_BIT_U32 or GENMASK_U32 or so in a generic kernel header so we don't
need to rely on the definition from i915.

>
>
>>  struct pagefault {
>>  	u64 page_addr;
>>  	u32 asid;
>> diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>> index c76aaea1887c..264d6429318b 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_topology.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>> @@ -10,6 +10,8 @@
>>  #include "xe_gt.h"
>>  #include "xe_mmio.h"
>>
>> +#include "../i915/i915_reg_defs.h"
>> +
>
>I think there are some registers defined at the top of this file from
>before we figured out what we were going to do about register
>definitions; those should probably migrate into the new register
>headers.  And then we can just include the register header instead of
>the reg_defs.

I was thinking about leaving the register definitions that are need in
just one compilation unit in that unit rather than moving to the header.
But that was before the i915 -> xe header transition. So, yeah... I
think we can just move them to the header.

>
>>  #define XE_MAX_DSS_FUSE_BITS (32 * XE_MAX_DSS_FUSE_REGS)
>>  #define XE_MAX_EU_FUSE_BITS (32 * XE_MAX_EU_FUSE_REGS)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index 6c0033cb238f..d845f7cadbd4 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -28,6 +28,8 @@
>>  #include "xe_trace.h"
>>  #include "xe_vm.h"
>>
>> +#include "../i915/i915_reg_defs.h"
>
>Maybe another candidate for linux/bitfield.h?
>
>> +
>>  /**
>>   * struct xe_migrate - migrate context.
>>   */
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 0762c8a912c7..2103f684a8b5 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -5,6 +5,8 @@
>>
>>  /* Internal to xe_pcode */
>>
>> +#include "../i915/i915_reg_defs.h"
>> +
>>  #define PCODE_MAILBOX			_MMIO(0x138124)
>>  #define   PCODE_READY			REG_BIT(31)
>>  #define   PCODE_MB_PARAM2		REG_GENMASK(23, 16)
>> diff --git a/drivers/gpu/drm/xe/xe_step.c b/drivers/gpu/drm/xe/xe_step.c
>> index ca77d0971529..e5e77cb9ea09 100644
>> --- a/drivers/gpu/drm/xe/xe_step.c
>> +++ b/drivers/gpu/drm/xe/xe_step.c
>> @@ -8,6 +8,8 @@
>>  #include "xe_device.h"
>>  #include "xe_platform_types.h"
>>
>> +#include "../i915/i915_reg_defs.h"
>
>And another linux/bitfield.h here too?

yeah, I will move those to use linux/bitfield.h

thanks
Lucas De Marchi

>
>
>Matt
>
>> +
>>  /*
>>   * Provide mapping between PCI's revision ID to the individual GMD
>>   * (Graphics/Media/Display) stepping values that can be compared numerically.
>> --
>> 2.39.0
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation