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

[Lucas De Marchi]

On Fri, Feb 24, 2023 at 10:20:10AM -0800, Matt Roper wrote:
>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.

yeah, I like xe_lrc_layout.h. I will change to use it.

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

yeah... sounds good. I will keep it as is for now since this series is a
prerequisite fro the WAs I'm working on. But changing to a struct layout
IMO would be a very good improvement.  Let me also add an issue in
gitlab with that so we don't forget.

thanks
Lucas De Marchi