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

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

On 23.07.2024 15:15, Nirmoy Das wrote:
> 
> 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.

TBH this doesn't convinced me, as our other entries are not duplicating
GT id in the output and likely someone can do other trick to see the id:

# grep . -r /sys/kernel/debug/dri/0/gt*/tlb_stats

/sys/kernel/debug/dri/0/gt0/tlb_stats: Requests sent: 212, received: 212
/sys/kernel/debug/dri/0/gt1/tlb_stats: Requests sent: 212, received: 212

> 
> 
>> 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_ */