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

["Lis, Tomasz" <tomasz.lis@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5309 bytes --]


On 08.04.2025 15:34, Michal Wajdeczko wrote:
> 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
will add
>> +	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 ;)

will switch order of operands.

a macro wouldn't be much better, unless it merges the enum name, ie. 
`XE_GUC_REGISTER_CONTEXT_ ## part_name`.

But I really don't like such techniques, as I then _always_ hear someone 
can't find the name and is confused.

-Tomasz

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

[-- Attachment #2: Type: text/html, Size: 6363 bytes --]