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

[Michal Wajdeczko]

On 08.06.2021 02:05, Daniele Ceraolo Spurio wrote:
> 
> 
> 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.

sure, we can add some of above helper macros, but not part of the ABI
definitions, but in fwif.h where we have other wrappers

and better to add them later as optional improvement, when all
refactoring dust settles down

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