On 31.03.2025 15:21, Tomasz Lis wrote:
> Some GuC messages are constructed with incrementing dword counter
> rather than referencing specific DWORDs, as described in GuC interface
> specification.
>
> This change introduces the definitions of DWORD numbers for parameters
> which will need to be referenced in a CTB parser to be added in a
> following patch. To ensure correctness of these DWORDs, verification
> in form of asserts was added to the message construction code.
>
> Signed-off-by: Tomasz Lis <[email protected]>
> ---
> drivers/gpu/drm/xe/abi/guc_actions_abi.h | 7 +++++++
> drivers/gpu/drm/xe/xe_guc_submit.c | 4 ++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> index 448afb86e05c..86bd4b092c7c 100644
> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> @@ -161,6 +161,13 @@ enum xe_guc_preempt_options {
> XE_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q = 0x8,
> };
>
> +enum xe_guc_register_context_multi_lrc_param_offsets {
> + XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_WQ_DESC = 5,
> + XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_WQ_BASE = 7,
> + XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_N_CHILDREN = 10,
> + XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_HWLRCA = 11
while likely we only need those 4 indices, IMO it would cleaner to
define all of them in ABI to reflect the whole message layout:
enum xe_guc_register_context_multi_lrc_message {
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_0_MBZ = 0,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_1_FLAGS,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_2_PARENT_CONTEXT,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_3_ENGINE_CLASS,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_4_ENGINE_SUBMIT_MASK,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_5_WQ_DESC_ADDR_LOWER,
XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_6_WQ_DESC_ADDR_UPPER,
...
> +};
> +
> enum xe_guc_report_status {
> XE_GUC_REPORT_STATUS_UNKNOWN = 0x0,
> XE_GUC_REPORT_STATUS_ACKED = 0x1,
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 31bc2022bfc2..3c8574e2c1f3 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -469,12 +469,16 @@ static void __register_mlrc_exec_queue(struct xe_guc
> *guc,
> action[len++] = info->context_idx;
> action[len++] = info->engine_class;
> action[len++] = info->engine_submit_mask;
> + xe_gt_assert(guc_to_gt(guc), len ==
> XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_WQ_DESC);
> action[len++] = info->wq_desc_lo;
> action[len++] = info->wq_desc_hi;
> + xe_gt_assert(guc_to_gt(guc), len ==
> XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_WQ_BASE);
> action[len++] = info->wq_base_lo;
> action[len++] = info->wq_base_hi;
> action[len++] = info->wq_size;
> + xe_gt_assert(guc_to_gt(guc), len ==
> XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_N_CHILDREN);
> action[len++] = q->width;
> + xe_gt_assert(guc_to_gt(guc), len ==
> XE_GUC_REGISTER_CONTEXT_MULTI_LRC_OFFS_HWLRCA);
> action[len++] = info->hwlrca_lo;
> action[len++] = info->hwlrca_hi;
>
hmm, it looks little paranoid, and pollute otherwise clean code,
so maybe either convert completely to index based message encoding:
action[.._FLAGS] = info->flags;
action[.._WQ_DESC_LO] = info->wq_desc_lo;
action[.._WQ_DESC_HI] = info->wq_desc_hi;
...
for (i = 0; i < q->width; ++i)
action[.._HWLRCA_LO + 1 + i] = ..;
action[.._HWLRCA_HI + 1 + i] = ..;
or assert those important entries at the end:
xe_gt_assert(gt, action[..WQ_DESC_LO] == info->wq_desc_lo);
xe_gt_assert(gt, action[..HWLRCA_LO] == info->hwlrca_lo);
@Matt, any other ideas or you don't care?
