Re: [PATCH 2/2] drm/xe: Create debugfs for tlb inval stats

[Nirmoy Das <nirmoy.das@intel.com>]

On 7/23/2024 2:14 PM, Michal Wajdeczko wrote:
>
> On 23.07.2024 13:16, Nirmoy Das wrote:
>> Create debugfs file for each GT to dump tlb sent/receive
>> stats.
>>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt_debugfs.c          | 9 +++++++++
>>   drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 7 +++++++
>>   drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h | 2 ++
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> index 5e7fd937917a..959d979927dd 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> @@ -17,6 +17,7 @@
>>   #include "xe_gt_mcr.h"
>>   #include "xe_gt_sriov_pf_debugfs.h"
>>   #include "xe_gt_sriov_vf_debugfs.h"
>> +#include "xe_gt_tlb_invalidation.h"
>>   #include "xe_gt_topology.h"
>>   #include "xe_hw_engine.h"
>>   #include "xe_lrc.h"
>> @@ -269,6 +270,13 @@ static int vecs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
>>   	return 0;
>>   }
>>   
>> +static int tlb_stats(struct xe_gt *gt, struct drm_printer *p)
>> +{
>> +	xe_gt_tlb_dump(gt, p);
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct drm_info_list debugfs_list[] = {
>>   	{"hw_engines", .show = xe_gt_debugfs_simple_show, .data = hw_engines},
>>   	{"force_reset", .show = xe_gt_debugfs_simple_show, .data = force_reset},
>> @@ -286,6 +294,7 @@ static const struct drm_info_list debugfs_list[] = {
>>   	{"default_lrc_bcs", .show = xe_gt_debugfs_simple_show, .data = bcs_default_lrc},
>>   	{"default_lrc_vcs", .show = xe_gt_debugfs_simple_show, .data = vcs_default_lrc},
>>   	{"default_lrc_vecs", .show = xe_gt_debugfs_simple_show, .data = vecs_default_lrc},
>> +	{"tlb_stats", .show = xe_gt_debugfs_simple_show, .data = tlb_stats},
>>   };
>>   
>>   void xe_gt_debugfs_register(struct xe_gt *gt)
>> diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> index f84717c1aafa..62a6f42b6c60 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
>> @@ -540,3 +540,10 @@ void xe_gt_tlb_invalidation_fence_fini(struct xe_gt_tlb_invalidation_fence *fenc
>>   {
>>   	xe_pm_runtime_put(gt_to_xe(fence->gt));
>>   }
>> +
>> +void xe_gt_tlb_dump(struct xe_gt *gt, struct drm_printer *p)
> if you make this function as returning "int" then you will be able to
> plug it directly into debugfs_list without the tlb_stats() wrapper


Sounds goodI will modify it.

>
>> +{
>> +	drm_printf(p, "GT%d, TLB Requests sent: %llu, received: %llu\n",
>> +		   gt->info.id, atomic64_read(&gt->tlb_invalidation.sent_count),
>> +		   atomic64_read(&gt->tlb_invalidation.received_count));
> printing the GT identifier (GT%d) is redundant as path to this debugfs
> entry already contains GT identifier:
>
> $ sudo cat  /sys/kernel/debug/dri/0/gt1/tlb_stats
>                                      ^^^
>
> GT1, TLB Requests sent: 212, received: 212
> ^^^

Agreed but I was thinking when we do "cat 
/sys/kernel/debug/dri/0/gt*/tlb_stats" then keeping GT ID info  is useful.


> btw, maybe 'last seqno' and 'default timeout' will be worth to be
> included in debugfs ?


I will add those too, should be useful.


Thanks,

Nirmoy

>
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
>> index a84065fa324c..f420029ec02d 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
>> @@ -10,6 +10,7 @@
>>   
>>   #include "xe_gt_tlb_invalidation_types.h"
>>   
>> +struct drm_printer;
>>   struct xe_gt;
>>   struct xe_guc;
>>   struct xe_vma;
>> @@ -36,4 +37,5 @@ xe_gt_tlb_invalidation_fence_wait(struct xe_gt_tlb_invalidation_fence *fence)
>>   	dma_fence_wait(&fence->base, false);
>>   }
>>   
>> +void xe_gt_tlb_dump(struct xe_gt *gt, struct drm_printer *p);
> missing empty separation line
>
>>   #endif	/* _XE_GT_TLB_INVALIDATION_ */