Re: [PATCH 04/10] drm/xe/guc: Add helpers for HXG messages

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

On 29.12.2023 22:08, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on czw [2023-gru-28 00:58:32 +0100]:
>> In addition to MMIO and CTB communication between the host driver
>> and the GUC firmware, we will start using GuC HXG message protocol
>> in communication between SR-IOV VFs and PF. Define helpers related
>> to HXG message protocol to minimize code duplication.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_guc_hxg_helpers.h | 107 ++++++++++++++++++++++++
>>  1 file changed, 107 insertions(+)
>>  create mode 100644 drivers/gpu/drm/xe/xe_guc_hxg_helpers.h
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_hxg_helpers.h b/drivers/gpu/drm/xe/xe_guc_hxg_helpers.h
>> new file mode 100644
>> index 000000000000..4dc080484e7a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_hxg_helpers.h
>> @@ -0,0 +1,107 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GUC_HXG_HELPERS_H_
>> +#define _XE_GUC_HXG_HELPERS_H_
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/types.h>
>> +
>> +#include "abi/guc_messages_abi.h"
>> +
>> +/**
>> + * hxg_sizeof - Queries size of the object or type (in HXG units).
>> + *
>> + * Asserts when actual size is not aligned to HXG unit (u32).
>> + *
>> + * Return: size in dwords (u32).
>> + */
>> +#define hxg_sizeof(T)	(sizeof(T) / sizeof(u32) + BUILD_BUG_ON_ZERO(sizeof(T) % sizeof(u32)))
>> +
>> +static inline const char *guc_hxg_type_to_string(unsigned int type)
>> +{
>> +	switch (type) {
>> +	case GUC_HXG_TYPE_REQUEST:
>> +		return "request";
>> +	case GUC_HXG_TYPE_FAST_REQUEST:
>> +		return "fast-request";
>> +	case GUC_HXG_TYPE_EVENT:
>> +		return "event";
>> +	case GUC_HXG_TYPE_NO_RESPONSE_BUSY:
>> +		return "busy";
>> +	case GUC_HXG_TYPE_NO_RESPONSE_RETRY:
>> +		return "retry";
>> +	case GUC_HXG_TYPE_RESPONSE_FAILURE:
>> +		return "failure";
>> +	case GUC_HXG_TYPE_RESPONSE_SUCCESS:
>> +		return "response";
>> +	default:
>> +		return "<invalid>";
>> +	}
>> +}
>> +
>> +static inline bool guc_hxg_type_is_action(unsigned int type)
>> +{
>> +	switch (type) {
>> +	case GUC_HXG_TYPE_REQUEST:
>> +	case GUC_HXG_TYPE_FAST_REQUEST:
>> +	case GUC_HXG_TYPE_EVENT:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static inline bool guc_hxg_type_is_reply(unsigned int type)
>> +{
>> +	switch (type) {
>> +	case GUC_HXG_TYPE_NO_RESPONSE_BUSY:
>> +	case GUC_HXG_TYPE_NO_RESPONSE_RETRY:
>> +	case GUC_HXG_TYPE_RESPONSE_FAILURE:
>> +	case GUC_HXG_TYPE_RESPONSE_SUCCESS:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static inline u32 guc_hxg_msg_encode_success(u32 *msg, u32 data0)
>> +{
>> +	msg[0] = FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
>> +		 FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_RESPONSE_SUCCESS) |
>> +		 FIELD_PREP(GUC_HXG_RESPONSE_MSG_0_DATA0, data0);
>> +
>> +	return GUC_HXG_RESPONSE_MSG_MIN_LEN;
>> +}
>> +
>> +static inline u32 guc_hxg_msg_encode_failure(u32 *msg, u32 error, u32 hint)
>> +{
>> +	msg[0] = FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
>> +		 FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_RESPONSE_FAILURE) |
>> +		 FIELD_PREP(GUC_HXG_FAILURE_MSG_0_HINT, hint) |
>> +		 FIELD_PREP(GUC_HXG_FAILURE_MSG_0_ERROR, error);
>> +
>> +	return GUC_HXG_FAILURE_MSG_LEN;
>> +}
>> +
>> +static inline u32 guc_hxg_msg_encode_busy(u32 *msg, u32 counter)
>> +{
>> +	msg[0] = FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
>> +		 FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_NO_RESPONSE_BUSY) |
>> +		 FIELD_PREP(GUC_HXG_BUSY_MSG_0_COUNTER, counter);
>> +
>> +	return GUC_HXG_BUSY_MSG_LEN;
>> +}
>> +
>> +static inline u32 guc_hxg_msg_encode_retry(u32 *msg, u32 reason)
>> +{
>> +	msg[0] = FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
>> +		 FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_NO_RESPONSE_RETRY) |
>> +		 FIELD_PREP(GUC_HXG_RETRY_MSG_0_REASON, reason);
>> +
>> +	return GUC_HXG_RETRY_MSG_LEN;
>> +}
> 
> Note to the 4 functions above ^^^^^:
> I know that naming convention says to use in this case guc_hxg_*
> However, these functions, by their content directly indicate that they
> are only h2g.
> I have a subjective feeling, which you can ignore, that they should be
> labeled in some way as h2g.

Note that from the driver (host) POV, by design, we will be encoding all
HXG messages always as H2G, so IMO we can stay with guc_hxg prefix.

> 
> But still:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> 
> 
>> +
>> +#endif
>> -- 
>> 2.25.1
>>
>