[Intel-xe] [PATCH v2 06/11] drm/xe: Remove dependency on intel_lrc_reg.h

[Matt Roper]

On Thu, Feb 16, 2023 at 04:52:21PM -0800, Lucas De Marchi wrote:
> Create regs/xe_lrc_regs.h file with all the registers used by the xe
> driver. Eventually the registers may be defined in a different way and
> since xe doesn't supported below gen12, the number of registers touched
> is much smaller, so create a new header.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_lrc_regs.h | 17 +++++++++++++++++
>  drivers/gpu/drm/xe/xe_execlist.c      |  2 +-
>  drivers/gpu/drm/xe/xe_guc_submit.c    |  3 +--
>  drivers/gpu/drm/xe/xe_lrc.c           |  2 +-
>  drivers/gpu/drm/xe/xe_ring_ops.c      |  2 +-
>  5 files changed, 21 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/xe/regs/xe_lrc_regs.h
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_lrc_regs.h b/drivers/gpu/drm/xe/regs/xe_lrc_regs.h
> new file mode 100644
> index 000000000000..e8a13fa6722b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/regs/xe_lrc_regs.h

This file isn't so much about "register definitions" as it is about
"register offsets in the context image."  Maybe we should name this a
bit differently to make that clear?  E.g., xe_lrc_offsets.h or
xe_lrc_layout.h or something.

> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_LRC_REGS_H_
> +#define _XE_LRC_REGS_H_
> +
> +#define CTX_CONTEXT_CONTROL		(0x02 + 1)
> +#define CTX_RING_HEAD			(0x04 + 1)
> +#define CTX_RING_TAIL			(0x06 + 1)
> +#define CTX_RING_START			(0x08 + 1)
> +#define CTX_RING_CTL			(0x0a + 1)
> +#define CTX_PDP0_UDW			(0x30 + 1)
> +#define CTX_PDP0_LDW			(0x32 + 1)

I'm wondering if it would be more clear/obvious if we laid this out as a
C structure rather than using defines?

        struct tgl_lrc_layout {
                u8      _lri1;
                u16     CONTEXT_CONTROL;
                u16     RING_TAIL;
                u16     RING_START;
                ...
        };

and then later just use code like

        offsetof(struct tgl_lrc_layout, RING_TAIL);

to find the offsets when needed.  This could cover not just the
simple/common subset of registers here, but also stuff like per-platform
stuff like MI_MODE, BB_OFFSET, GPR0, etc. that we have lookup helpers
for in i915.

Anyway, that's something we can think about in the future; no need to
handle it as part of this series.


Matt

> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
> index b426f8000070..ea551dc922cc 100644
> --- a/drivers/gpu/drm/xe/xe_execlist.c
> +++ b/drivers/gpu/drm/xe/xe_execlist.c
> @@ -7,6 +7,7 @@
>  #include <drm/drm_managed.h>
>  
>  #include "regs/xe_engine_regs.h"
> +#include "regs/xe_lrc_regs.h"
>  #include "regs/xe_gt_regs.h"
>  #include "xe_bo.h"
>  #include "xe_device.h"
> @@ -21,7 +22,6 @@
>  #include "xe_sched_job.h"
>  
>  #include "gt/intel_gpu_commands.h"
> -#include "gt/intel_lrc_reg.h"
>  #include "i915_reg.h"
>  
>  #define XE_EXECLIST_HANG_LIMIT 1
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 305a0e061778..451c9e1c43ff 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -12,6 +12,7 @@
>  
>  #include <drm/drm_managed.h>
>  
> +#include "regs/xe_lrc_regs.h"
>  #include "xe_device.h"
>  #include "xe_engine.h"
>  #include "xe_force_wake.h"
> @@ -30,8 +31,6 @@
>  #include "xe_trace.h"
>  #include "xe_vm.h"
>  
> -#include "gt/intel_lrc_reg.h"
> -
>  static struct xe_gt *
>  guc_to_gt(struct xe_guc *guc)
>  {
> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index 8e341d11f2e3..af6fe47a0495 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -5,6 +5,7 @@
>  #include "xe_lrc.h"
>  
>  #include "regs/xe_engine_regs.h"
> +#include "regs/xe_lrc_regs.h"
>  #include "regs/xe_gt_regs.h"
>  #include "xe_bo.h"
>  #include "xe_device.h"
> @@ -15,7 +16,6 @@
>  #include "xe_vm.h"
>  
>  #include "gt/intel_gpu_commands.h"
> -#include "gt/intel_lrc_reg.h"
>  #include "i915_reg.h"
>  
>  #define GEN8_CTX_VALID				(1 << 0)
> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> index a7ab0d4451f0..0a1e32ab9758 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> @@ -4,6 +4,7 @@
>   */
>  #include "xe_ring_ops.h"
>  
> +#include "regs/xe_lrc_regs.h"
>  #include "regs/xe_gt_regs.h"
>  #include "xe_engine_types.h"
>  #include "xe_gt.h"
> @@ -13,7 +14,6 @@
>  #include "xe_vm_types.h"
>  
>  #include "gt/intel_gpu_commands.h"
> -#include "gt/intel_lrc_reg.h"
>  #include "i915_reg.h"
>  
>  static u32 preparser_disable(bool state)
> -- 
> 2.39.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation