Re: [PATCH 1/2] drm/xe/guc: Use HXG definitions on HXG messages

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

On 11.01.2024 00:06, Matthew Brost wrote:
> On Wed, Jan 10, 2024 at 08:59:50PM +0100, Michal Wajdeczko wrote:
>> While parsing and processing CTB G2H messages we should extract
>> underlying HXG message and use HXG definitions on such message.
>> Using outer CTB layer message in HXG definitions require use of
>> shifted dword index, which might be confusing:
>>
>> 	FIELD_GET(GUC_HXG_MSG_0_xxx, msg[1])
>>
>> instead of:
>>
>> 	FIELD_GET(GUC_HXG_MSG_0_xxx, hxg[0])
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_guc_ct.c | 72 +++++++++++++++++++++-------------
>>  1 file changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>> index c29f095aa1b9..d6b7020a2d2f 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>> @@ -796,9 +796,20 @@ int xe_guc_ct_send_recv_no_fail(struct xe_guc_ct *ct, const u32 *action,
>>  	return guc_ct_send_recv(ct, action, len, response_buffer, true);
>>  }
>>  
>> +static u32 *msg_to_hxg(u32 *msg)
>> +{
>> +	return msg + GUC_CTB_MSG_MIN_LEN;
>> +}
>> +
>> +static u32 msg_len_to_hxg(u32 len)
> 
> msg_len_to_hxg_len?

ok

> 
>> +{
>> +	return len - GUC_CTB_MSG_MIN_LEN;
>> +}
>> +
>>  static int parse_g2h_event(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  {
>> -	u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> +	u32 *hxg = msg_to_hxg(msg);
>> +	u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>>  
>>  	lockdep_assert_held(&ct->lock);
>>  
>> @@ -817,9 +828,10 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  {
>>  	struct xe_gt *gt =  ct_to_gt(ct);
>>  	struct xe_device *xe = gt_to_xe(gt);
>> -	u32 response_len = len - GUC_CTB_MSG_MIN_LEN;
>> +	const u32 *hxg = msg_to_hxg(msg);
> 
> Some places do:
> const u32 *hxg = msg_to_hxg(msg);
> 
> Others do:
> u32 *hxg = msg_to_hxg(msg);
> 
> I do not see any reason why we have 2 different conventions.

I was trying to add constness concept of parsed/processed HXG message,
but since it wasn't done correctly for passed CTB message arguments I
gave up, missed to cleanup this one leftover

will drop const for now

> 
> Everything else LGTM.
> 
> Matt
> 
>> +	u32 hxg_len = msg_len_to_hxg(len);
>>  	u32 fence = FIELD_GET(GUC_CTB_MSG_0_FENCE, msg[0]);
>> -	u32 type = FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]);
>> +	u32 type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]);
>>  	struct g2h_fence *g2h_fence;
>>  
>>  	lockdep_assert_held(&ct->lock);
>> @@ -836,8 +848,8 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  		if (type == GUC_HXG_TYPE_RESPONSE_FAILURE)
>>  			xe_gt_err(gt, "FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
>>  				  fence,
>> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, msg[1]),
>> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, msg[1]));
>> +				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
>> +				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
>>  		else
>>  			xe_gt_err(gt, "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
>>  				  type, fence);
>> @@ -857,18 +869,14 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  
>>  	if (type == GUC_HXG_TYPE_RESPONSE_FAILURE) {
>>  		g2h_fence->fail = true;
>> -		g2h_fence->error =
>> -			FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, msg[1]);
>> -		g2h_fence->hint =
>> -			FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, msg[1]);
>> +		g2h_fence->error = FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]);
>> +		g2h_fence->hint = FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]);
>>  	} else if (type == GUC_HXG_TYPE_NO_RESPONSE_RETRY) {
>>  		g2h_fence->retry = true;
>> -		g2h_fence->reason =
>> -			FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, msg[1]);
>> +		g2h_fence->reason = FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, hxg[0]);
>>  	} else if (g2h_fence->response_buffer) {
>> -		g2h_fence->response_len = response_len;
>> -		memcpy(g2h_fence->response_buffer, msg + GUC_CTB_MSG_MIN_LEN,
>> -		       response_len * sizeof(u32));
>> +		g2h_fence->response_len = hxg_len;
>> +		memcpy(g2h_fence->response_buffer, hxg, hxg_len * sizeof(u32));
>>  	}
>>  
>>  	g2h_release_space(ct, GUC_CTB_HXG_MSG_MAX_LEN);
>> @@ -884,14 +892,13 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  {
>>  	struct xe_device *xe = ct_to_xe(ct);
>> -	u32 hxg, origin, type;
>> +	const u32 *hxg = msg_to_hxg(msg);
>> +	u32 origin, type;
>>  	int ret;
>>  
>>  	lockdep_assert_held(&ct->lock);
>>  
>> -	hxg = msg[1];
>> -
>> -	origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg);
>> +	origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]);
>>  	if (unlikely(origin != GUC_HXG_ORIGIN_GUC)) {
>>  		drm_err(&xe->drm,
>>  			"G2H channel broken on read, origin=%d, reset required\n",
>> @@ -901,7 +908,7 @@ static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  		return -EPROTO;
>>  	}
>>  
>> -	type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg);
>> +	type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]);
>>  	switch (type) {
>>  	case GUC_HXG_TYPE_EVENT:
>>  		ret = parse_g2h_event(ct, msg, len);
>> @@ -927,14 +934,19 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  {
>>  	struct xe_device *xe = ct_to_xe(ct);
>>  	struct xe_guc *guc = ct_to_guc(ct);
>> -	u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> -	u32 *payload = msg + GUC_CTB_HXG_MSG_MIN_LEN;
>> -	u32 adj_len = len - GUC_CTB_HXG_MSG_MIN_LEN;
>> +	u32 hxg_len = msg_len_to_hxg(len);
>> +	u32 *hxg = msg_to_hxg(msg);
>> +	u32 action, adj_len;
>> +	u32 *payload;
>>  	int ret = 0;
>>  
>> -	if (FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]) != GUC_HXG_TYPE_EVENT)
>> +	if (FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_EVENT)
>>  		return 0;
>>  
>> +	action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>> +	payload = hxg + GUC_HXG_EVENT_MSG_MIN_LEN;
>> +	adj_len = hxg_len - GUC_HXG_EVENT_MSG_MIN_LEN;
>> +
>>  	switch (action) {
>>  	case XE_GUC_ACTION_SCHED_CONTEXT_MODE_DONE:
>>  		ret = xe_guc_sched_done_handler(guc, payload, adj_len);
>> @@ -995,6 +1007,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>  	u32 tail, head, len;
>>  	s32 avail;
>>  	u32 action;
>> +	u32 *hxg;
>>  
>>  	lockdep_assert_held(&ct->fast_lock);
>>  
>> @@ -1045,10 +1058,11 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>  				   avail * sizeof(u32));
>>  	}
>>  
>> -	action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> +	hxg = msg_to_hxg(msg);
>> +	action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>>  
>>  	if (fast_path) {
>> -		if (FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]) != GUC_HXG_TYPE_EVENT)
>> +		if (FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_EVENT)
>>  			return 0;
>>  
>>  		switch (action) {
>> @@ -1074,9 +1088,11 @@ static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>  {
>>  	struct xe_device *xe = ct_to_xe(ct);
>>  	struct xe_guc *guc = ct_to_guc(ct);
>> -	u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> -	u32 *payload = msg + GUC_CTB_HXG_MSG_MIN_LEN;
>> -	u32 adj_len = len - GUC_CTB_HXG_MSG_MIN_LEN;
>> +	u32 hxg_len = msg_len_to_hxg(len);
>> +	u32 *hxg = msg_to_hxg(msg);
>> +	u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>> +	u32 *payload = hxg + GUC_HXG_MSG_MIN_LEN;
>> +	u32 adj_len = hxg_len - GUC_HXG_MSG_MIN_LEN;
>>  	int ret = 0;
>>  
>>  	switch (action) {
>>
>> base-commit: bb9e8031d2feb59becdea41e54e62f1bc47f3ef9
>> -- 
>> 2.25.1
>>