Re: [PATCH 2/2] drm/xe/guc: Fix arguments passed to relay G2H handlers

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

On 11.01.2024 21:08, Matthew Brost wrote:
> On Thu, Jan 11, 2024 at 10:37:31AM +0100, Michal Wajdeczko wrote:
>>
>>
>> On 11.01.2024 00:07, Matthew Brost wrote:
>>> On Wed, Jan 10, 2024 at 08:59:51PM +0100, Michal Wajdeczko wrote:
>>>> By default CT code was passing just payload of the G2H event
>>>> message, while Relay code expects full G2H message including
>>>> HXG header which contains DATA0 field. Fix that.
>>>>
>>>> Fixes: 152577060697 ("drm/xe/guc: Start handling GuC Relay event messages")
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>
>>> FWIW I do think the argument names in xe_guc_relay_process_* functions
>>> should be changed but not going to hold of this fix.
>>
>> I can rename message argument in relay g2h handlers to "hxg", but what
>> about the other g2h handlers, which (IMO wrongly) take just payload, not
>> a message, while still use "msg" as an argument name:
>>
>> int xe_guc_sched_done_handler(... u32 *msg, u32 len)
>> int xe_guc_deregister_done_handler(... u32 *msg, u32 len)
>> int xe_guc_exec_queue_reset_handler(... u32 *msg, u32 len)
>> int xe_guc_exec_queue_memory_cat_error_handler(... u32 *msg, u32 len)
>> int xe_guc_exec_queue_reset_failure_handler(... u32 *msg, u32 len)
>> int xe_guc_pagefault_handler(... u32 *msg, u32 len)
>> int xe_guc_tlb_invalidation_done_handler(... u32 *msg, u32 len)
>> int xe_guc_access_counter_notify_handler(... u32 *msg, u32 len)
>>
> 
> Good point I'd say lets clean all of this up.
> 
> msg -> entire G2H
> hxg -> HXG + payload
> payload -> payload

but maybe we can limit this notation only to CT code as only there this
different naming is needed - all CT users will be using just one kind of
the message and it will be HXG message (with payload) by design

also actual actions definitions [1] [2] don't use HXG tag while
describing full action/response message fields, while there is MSG tag

so IMO using "msg" in G2H handlers (code outside CT) seems to be fine

[1]
https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/abi/guc_actions_abi.h?ref_type=heads
[2]
https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/abi/guc_actions_sriov_abi.h?ref_type=heads

> 
> Ok with a follow up. We can chat off the list with who will post.

as stated above passing just payload to G2H handlers seems wrong as then
data0 is never passed on (well, it's MBZ for most actions now, but we
don't even check if this is the case and are unable to react if not)

so sooner or later, that "msg" which currently means "payload" will mean
"full hxg" - the only kind message of message used beyond CT layer - and
the "payload only" concept will fully disappear - so I'm not sure that
just renaming functions arguments and hide the problem is a way to go

> 
> Matt
> 
>>>
>>> With that:
>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>>
>>>> ---
>>>>  drivers/gpu/drm/xe/xe_guc_ct.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> index d6b7020a2d2f..4a0c9ce13bf8 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> @@ -984,10 +984,10 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>>>  							   adj_len);
>>>>  		break;
>>>>  	case XE_GUC_ACTION_GUC2PF_RELAY_FROM_VF:
>>>> -		ret = xe_guc_relay_process_guc2pf(&guc->relay, payload, adj_len);
>>>> +		ret = xe_guc_relay_process_guc2pf(&guc->relay, hxg, hxg_len);
>>>>  		break;
>>>>  	case XE_GUC_ACTION_GUC2VF_RELAY_FROM_PF:
>>>> -		ret = xe_guc_relay_process_guc2vf(&guc->relay, payload, adj_len);
>>>> +		ret = xe_guc_relay_process_guc2vf(&guc->relay, hxg, hxg_len);
>>>>  		break;
>>>>  	default:
>>>>  		drm_err(&xe->drm, "unexpected action 0x%04x\n", action);
>>>> -- 
>>>> 2.25.1
>>>>