[Intel-gfx] [PATCH 03/13] drm/i915/guc: Update CTB response status definition

[Daniele Ceraolo Spurio]

On 6/7/2021 11:03 AM, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko at intel.com>
>
> Format of the STATUS dword in CTB response message now follows
> definition of the HXG header. Update our code and remove any
> obsolete legacy definitions.

This is kind of hard to review on its own against the specs, because 
there are larger changes to the CTB flows which AFAICS are part of patch 
8. If what you wanted to do here was a simple replacement of defines to 
keep the later patch simpler, then, considering all patches are going to 
be squashed anyway:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

One suggestion below.

>
> GuC: 55.0.0
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Acked-by: Piotr Piórkowski <piotr.piorkowski at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c   | 14 ++++++++------
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 17 -----------------
>   2 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index 8f7b148fef58..3f7f48611487 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -477,7 +477,9 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
>   	 * up to that length of time, then switch to a slower sleep-wait loop.
>   	 * No GuC command should ever take longer than 10ms.
>   	 */
> -#define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
> +#define done \
> +	(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \
> +	 GUC_HXG_ORIGIN_GUC)
>   	err = wait_for_us(done, 10);
>   	if (err)
>   		err = wait_for(done, 10);
> @@ -532,21 +534,21 @@ static int ct_send(struct intel_guc_ct *ct,
>   	if (unlikely(err))
>   		goto unlink;
>   
> -	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) {
> +	if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != GUC_HXG_TYPE_RESPONSE_SUCCESS) {
>   		err = -EIO;
>   		goto unlink;
>   	}
>   
>   	if (response_buf) {
>   		/* There shall be no data in the status */
> -		WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status));
> +		WARN_ON(FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, request.status));
>   		/* Return actual response len */
>   		err = request.response_len;
>   	} else {
>   		/* There shall be no response payload */
>   		WARN_ON(request.response_len);
>   		/* Return data decoded from the status dword */
> -		err = INTEL_GUC_MSG_TO_DATA(*status);
> +		err = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, *status);

Given that the same FIELD_GET() are repeated multiple times, IMO we 
could've kept some helper macros, something like:

INTEL_GUC_HXG_RESPONSE_TO_DATA(hxg) \
	FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, hxg)

INTEL_GUC_HXG_ORIGIN_IS_GUC(hxg) \
	(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg) == GUC_HXG_ORIGIN_GUC)

INTEL_GUC_HXG_TYPE(hxg) \
	FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg)

Which could be useful in the mmio code as well.
Not sure how this changes in patch 8 though, I might put some more 
comments on that patch.

Daniele

>   	}
>   
>   unlink:
> @@ -741,8 +743,8 @@ static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *r
>   	status = response->msg[2];
>   	datalen = len - 2;
>   
> -	/* Format of the status follows RESPONSE message */
> -	if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) {
> +	/* Format of the status dword follows HXG header */
> +	if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, status) != GUC_HXG_ORIGIN_GUC)) {
>   		CT_ERROR(ct, "Corrupted response (status %#x)\n", status);
>   		return -EPROTO;
>   	}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index e9a9d85e2aa3..fb04e2211b79 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -414,23 +414,6 @@ struct guc_shared_ctx_data {
>   	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>   } __packed;
>   
> -#define __INTEL_GUC_MSG_GET(T, m) \
> -	(((m) & INTEL_GUC_MSG_ ## T ## _MASK) >> INTEL_GUC_MSG_ ## T ## _SHIFT)
> -#define INTEL_GUC_MSG_TO_TYPE(m)	__INTEL_GUC_MSG_GET(TYPE, m)
> -#define INTEL_GUC_MSG_TO_DATA(m)	__INTEL_GUC_MSG_GET(DATA, m)
> -#define INTEL_GUC_MSG_TO_CODE(m)	__INTEL_GUC_MSG_GET(CODE, m)
> -
> -#define __INTEL_GUC_MSG_TYPE_IS(T, m) \
> -	(INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_ ## T)
> -#define INTEL_GUC_MSG_IS_REQUEST(m)	__INTEL_GUC_MSG_TYPE_IS(REQUEST, m)
> -#define INTEL_GUC_MSG_IS_RESPONSE(m)	__INTEL_GUC_MSG_TYPE_IS(RESPONSE, m)
> -
> -#define INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(m) \
> -	 (typecheck(u32, (m)) && \
> -	  ((m) & (INTEL_GUC_MSG_TYPE_MASK | INTEL_GUC_MSG_CODE_MASK)) == \
> -	  ((INTEL_GUC_MSG_TYPE_RESPONSE << INTEL_GUC_MSG_TYPE_SHIFT) | \
> -	   (INTEL_GUC_RESPONSE_STATUS_SUCCESS << INTEL_GUC_MSG_CODE_SHIFT)))
> -
>   /* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
>   enum intel_guc_recv_message {
>   	INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED = BIT(1),