Re: [Intel-xe] [PATCH 1/2] drm/xe: Add event tracing for CTB

["Kalvala, Haridhar" <haridhar.kalvala@intel.com>]

On 10/19/2023 1:15 PM, Balasubramani Vivekanandan wrote:
> On 18.10.2023 13:19, Kalvala, Haridhar wrote:
>> On 10/16/2023 3:47 PM, Balasubramani Vivekanandan wrote:
>>> Event tracing enabled for CTB submissions.
>>>
>>> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Looks good to me

Reviewed-by: Haridhar Kalvala <haridhar.kalvala@intel.com>

>>> ---
>>>    drivers/gpu/drm/xe/xe_guc_ct.c | 13 ++++++++--
>>>    drivers/gpu/drm/xe/xe_trace.h  | 43 ++++++++++++++++++++++++++++++++++
>>>    2 files changed, 54 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>> index 8b686c8b3339..a84e111bb36a 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>>> @@ -460,7 +460,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>>    	/* Write H2G ensuring visable before descriptor update */
>>>    	xe_map_memcpy_to(xe, &map, 0, cmd, H2G_CT_HEADERS * sizeof(u32));
>>>    	xe_map_memcpy_to(xe, &map, H2G_CT_HEADERS * sizeof(u32), action, len * sizeof(u32));
>>> -	xe_device_wmb(ct_to_xe(ct));
>>> +	xe_device_wmb(xe);
>> Hi Bala, I think its better to describe this change in commit message
>> (ct_to_xe(ct)  -> xe).
> Done
>
>>>    	/* Update local copies */
>>>    	h2g->info.tail = (tail + full_len) % h2g->info.size;
>>> @@ -469,6 +469,9 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>>    	/* Update descriptor */
>>>    	desc_write(xe, h2g, tail, h2g->info.tail);
>>> +	trace_xe_guc_ctb_h2g(ct_to_gt(ct)->info.id, *(action - 1), full_len,
>>> +			     desc_read(xe, h2g, head), h2g->info.tail);
>>> +
>>>    	return 0;
>>>    }
>>> @@ -934,6 +937,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>>    	struct guc_ctb *g2h = &ct->ctbs.g2h;
>>>    	u32 tail, head, len;
>>>    	s32 avail;
>>> +	u32 action;
>>>    	lockdep_assert_held(&ct->fast_lock);
>>> @@ -984,11 +988,13 @@ 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]);
>>> +
>>>    	if (fast_path) {
>>>    		if (FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]) != GUC_HXG_TYPE_EVENT)
>>>    			return 0;
>>> -		switch (FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1])) {
>> action can be assigned here instead before condition to return.
> action is also used outside the fast_path condition check. So if it is
> assigned inside the condition block, it would remain unassigned outside
> the condition block if fast_path is not true.
>
>>> +		switch (action) {
>>>    		case XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC:
>>>    		case XE_GUC_ACTION_TLB_INVALIDATION_DONE:
>>>    			break;	/* Process these in fast-path */
>>> @@ -1001,6 +1007,9 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>>    	g2h->info.head = (head + avail) % g2h->info.size;
>>>    	desc_write(xe, g2h, head, g2h->info.head);
>>> +	trace_xe_guc_ctb_g2h(ct_to_gt(ct)->info.id, action, len,
>>> +			     g2h->info.head, tail);
>>> +
>>>    	return len;
>>>    }
>>> diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h
>>> index e32f1cad51d9..456c776f4be1 100644
>>> --- a/drivers/gpu/drm/xe/xe_trace.h
>>> +++ b/drivers/gpu/drm/xe/xe_trace.h
>>> @@ -520,6 +520,7 @@ DEFINE_EVENT(xe_vm, xe_vm_rebind_worker_exit,
>>>    TRACE_EVENT(xe_guc_ct_h2g_flow_control,
>>>    	    TP_PROTO(u32 _head, u32 _tail, u32 size, u32 space, u32 len),
>>>    	    TP_ARGS(_head, _tail, size, space, len),
>>> +/* GUC */
>>>    	    TP_STRUCT__entry(
>>>    		     __field(u32, _head)
>>> @@ -567,6 +568,48 @@ TRACE_EVENT(xe_guc_ct_g2h_flow_control,
>>>    		      __entry->space, __entry->len)
>>>    );
>>> +DECLARE_EVENT_CLASS(xe_guc_ctb,
>>> +		    TP_PROTO(u8 gt_id, u32 action, u32 len, u32 _head, u32 tail),
>>> +		    TP_ARGS(gt_id, action, len, _head, tail),
>>> +
>>> +		    TP_STRUCT__entry(
>>> +				__field(u8, gt_id)
>>> +				__field(u32, action)
>>> +				__field(u32, len)
>>> +				__field(u32, tail)
>>> +				__field(u32, _head)
>>> +		    ),
>>> +
>>> +		    TP_fast_assign(
>>> +			    __entry->gt_id = gt_id;
>>> +			    __entry->action = action;
>>> +			    __entry->len = len;
>>> +			    __entry->tail = tail;
>>> +			    __entry->_head = _head;
>>> +		    ),
>>> +
>>> +		    TP_printk("gt%d: H2G CTB: action=0x%x, len=%d, tail=%d, head=%d\n",
>>> +			      __entry->gt_id, __entry->action, __entry->len,
>>> +			      __entry->tail, __entry->_head)
>>> +);
>>> +
>>> +DEFINE_EVENT(xe_guc_ctb, xe_guc_ctb_h2g,
>>> +	     TP_PROTO(u8 gt_id, u32 action, u32 len, u32 _head, u32 tail),
>>> +	     TP_ARGS(gt_id, action, len, _head, tail)
>>> +);
>>> +
>>> +DEFINE_EVENT_PRINT(xe_guc_ctb, xe_guc_ctb_g2h,
>>> +		   TP_PROTO(u8 gt_id, u32 action, u32 len, u32 _head, u32 tail),
>>> +		   TP_ARGS(gt_id, action, len, _head, tail),
>>> +
>>> +		   TP_printk("gt%d: G2H CTB: action=0x%x, len=%d, tail=%d, head=%d\n",
>>> +			     __entry->gt_id, __entry->action, __entry->len,
>>> +			     __entry->tail, __entry->_head)
>>> +
>>> +);
>>> +
>>> +/* GuC */
>> I think this comment not required at end.
> Done
>
> Regards,
> Bala
>
>>> +
>>>    #endif
>>>    /* This part must be outside protection */
>> -- 
>> Regards,
>> Haridhar Kalvala
>>
-- 
Regards,
Haridhar Kalvala