[PATCH v6 3/4] drm/xe/guc: Introduce enum with offsets for multi-LRC register H2G

[Lis, Tomasz]

On 01.04.2025 19:24, Michal Wajdeczko wrote:
> 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<tomasz.lis at intel.com>
>> ---
>>   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,
> 	...

Will agree to all the propositions here as this whole patch isn't my idea.

-Tomasz

>> +};
>> +
>>   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?
>
>