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

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

On 7/23/2024 4:27 PM, Michal Wajdeczko wrote:
>
> 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


Yes, I see your point. If other entries are not duplicating it then it 
doesn't make sense.


Thanks,

Nirmoy

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