Re: [PATCH v7 3/4] drm/xe/guc: Introduce enum with offsets for context register H2Gs

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 03.04.2025 20:40, 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.
> 
> v2: Renamed enum members, added ones for single context registration,
>   modified asserts to check values rather than indexes.
> 
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/xe/abi/guc_actions_abi.h | 29 ++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc_submit.c       | 22 ++++++++++++++++++
>  2 files changed, 51 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..64c71526356e 100644
> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> @@ -161,6 +161,35 @@ enum xe_guc_preempt_options {
>  	XE_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q = 0x8,
>  };
>  
> +enum xe_guc_register_context_param_offsets {
> +	XE_GUC_REGISTER_CONTEXT_DATA_0_MBZ = 0,
> +	XE_GUC_REGISTER_CONTEXT_DATA_1_FLAGS,
> +	XE_GUC_REGISTER_CONTEXT_DATA_2_CONTEXT_INDEX,
> +	XE_GUC_REGISTER_CONTEXT_DATA_3_ENGINE_CLASS,
> +	XE_GUC_REGISTER_CONTEXT_DATA_4_ENGINE_SUBMIT_MASK,
> +	XE_GUC_REGISTER_CONTEXT_DATA_5_WQ_DESC_ADDR_LOWER,
> +	XE_GUC_REGISTER_CONTEXT_DATA_6_WQ_DESC_ADDR_UPPER,
> +	XE_GUC_REGISTER_CONTEXT_DATA_7_WQ_BUF_BASE_LOWER,
> +	XE_GUC_REGISTER_CONTEXT_DATA_8_WQ_BUF_BASE_UPPER,
> +	XE_GUC_REGISTER_CONTEXT_DATA_9_WQ_BUF_SIZE,
> +	XE_GUC_REGISTER_CONTEXT_DATA_10_HW_LRC_ADDR,
> +};
> +
> +enum xe_guc_register_context_multi_lrc_param_offsets {
> +	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,
> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_7_WQ_BUF_BASE_LOWER,
> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_8_WQ_BUF_BASE_UPPER,
> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_9_WQ_BUF_SIZE,
> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_10_NUM_CTXS,
> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_11_HW_LRC_ADDR,
> +};
> +
>  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..63ef06d3a28f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -485,6 +485,18 @@ static void __register_mlrc_exec_queue(struct xe_guc *guc,
>  		action[len++] = upper_32_bits(xe_lrc_descriptor(lrc));
>  	}
>  

nit: if we want to keep these asserts then small comment saying

	/* explicitly checks some fields that we might fixup later */

will not hurt

> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_5_WQ_DESC_ADDR_LOWER]
> +		     == info->wq_desc_lo);
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_7_WQ_BUF_BASE_LOWER]
> +		     == info->wq_base_lo);
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_10_NUM_CTXS]
> +		     == q->width);
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_11_HW_LRC_ADDR]
> +		     == info->hwlrca_lo);

maybe we can spare one line in each assert by:

xe_gt_assert(guc_to_gt(guc), info->hwlrca_lo ==
	action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_11_HW_LRC_ADDR]);

or by introducing:

	xe_guc_assert(guc, cond)

or by ignoring 100 column limit - it will not be first time in xe ;)

>  	xe_gt_assert(guc_to_gt(guc), len <= MAX_MLRC_REG_SIZE);
>  #undef MAX_MLRC_REG_SIZE
>  
> @@ -509,6 +521,16 @@ static void __register_exec_queue(struct xe_guc *guc,
>  		info->hwlrca_hi,
>  	};
>  
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_5_WQ_DESC_ADDR_LOWER]
> +		     == info->wq_desc_lo);
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_7_WQ_BUF_BASE_LOWER]
> +		     == info->wq_base_lo);
> +	xe_gt_assert(guc_to_gt(guc),
> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_10_HW_LRC_ADDR]
> +		     == info->hwlrca_lo);
> +
>  	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
>  }
>  

otherwise, since I don't have better ideas for enum names, LGTM