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

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

On 23.04.2025 10:35, Michal Wajdeczko wrote:
>
> On 18.04.2025 16:10, 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.
>> v3: Reordered assert args to take less lines
>> v4: Added lengths
>>
>> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> (v3)
>> ---
>>   drivers/gpu/drm/xe/abi/guc_actions_abi.h | 31 ++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_submit.c       | 17 +++++++++++++
>>   2 files changed, 48 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..3c2808fabc6a 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> @@ -161,6 +161,37 @@ 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,
>> +	XE_GUC_REGISTER_CONTEXT_MSG_LEN,
> didn't notice this earlier...
>
> this enum, as it's name says, was about param offsets, so adding here
> MSG_LEN is not the best fit, likely should be a separate #define, unless
> we want to say that this enum is just a placeholder for random defines
> related to this H2G action ... it's just sad that we don't have (and
> don't want) unified way how to describe GuC ABI messages consts

Using last enum entry to get length is a commonly used trope. This is 
not random, enums by design invite the opportunity for such define.

Will leave as is.

Though I won't have an issue with this changing, if we ever reach that 
unified way and we decide to keep the MAX_LEN outside of the enum.

>> +};
>> +
>> +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,
>> +	XE_GUC_REGISTER_CONTEXT_MULTI_LRC_MSG_LEN = 11,
> same here and also this one should be named, as already suggested, as
> "MSG_MIN_LEN" since this message has a flexible length and we can be
> only sure about its min size (or max if we consider max CTB msg len)
Will rename.
>
>> +};
>> +
>>   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 813c3c0bb250..cfc65f21b2f7 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -487,6 +487,15 @@ static void __register_mlrc_exec_queue(struct xe_guc *guc,
>>   		action[len++] = upper_32_bits(xe_lrc_descriptor(lrc));
>>   	}
>>   
>> +	/* explicitly checks some fields that we might fixup later */
>> +	xe_gt_assert(guc_to_gt(guc), info->wq_desc_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_5_WQ_DESC_ADDR_LOWER]);
>> +	xe_gt_assert(guc_to_gt(guc), info->wq_base_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_7_WQ_BUF_BASE_LOWER]);
>> +	xe_gt_assert(guc_to_gt(guc), q->width ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_10_NUM_CTXS]);
>> +	xe_gt_assert(guc_to_gt(guc), info->hwlrca_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_MULTI_LRC_DATA_11_HW_LRC_ADDR]);
>>   	xe_gt_assert(guc_to_gt(guc), len <= MAX_MLRC_REG_SIZE);
>>   #undef MAX_MLRC_REG_SIZE
>>   
>> @@ -511,6 +520,14 @@ static void __register_exec_queue(struct xe_guc *guc,
>>   		info->hwlrca_hi,
>>   	};
> nit: since v4 we also have MSG_LEN so we can use it in action[]

You mean assert for ARRAY_SIZE(action)?  Makes sense, but the 
initialization within declaration will get removed anyway.

Why - because we will add GGTT address locking in one of upcoming 
series, and we will have to lock before the action[] is filled.

No need to pretty a code which can't stay.

>>   
>> +	/* explicitly checks some fields that we might fixup later */
>> +	xe_gt_assert(guc_to_gt(guc), info->wq_desc_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_5_WQ_DESC_ADDR_LOWER]);
>> +	xe_gt_assert(guc_to_gt(guc), info->wq_base_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_7_WQ_BUF_BASE_LOWER]);
>> +	xe_gt_assert(guc_to_gt(guc), info->hwlrca_lo ==
>> +		     action[XE_GUC_REGISTER_CONTEXT_DATA_10_HW_LRC_ADDR]);
>> +
>>   	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
>>   }
>>   
> as above comments are just about enum names, which could be updated once
> we decide on how to unify all GuC ABI definitions, this is not worth
> blocking the whole series, so let it be,
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Thanks.

-Tomasz