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

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

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.

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


> +
> +#endif
> -- 
> 2.25.1
> 

--