Re: [PATCH 3/3] drm/xe/client: Skip drm-total-cycles if unable to read timestamp

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 2/4/2025 9:13 PM, Lucas De Marchi wrote:
> On Tue, Feb 04, 2025 at 07:56:03PM +0100, Michal Wajdeczko wrote:
>>
>>
>> On 04.02.2025 19:05, Marcin Bernatowicz wrote:
>>> Ensure show_run_ticks() only prints drm-total-cycles when timestamp
>>> retrieval succeeds.
>>>
>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_drm_client.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/ 
>>> xe_drm_client.c
>>> index 63f30b6df70b..e5c4f342380e 100644
>>> --- a/drivers/gpu/drm/xe/xe_drm_client.c
>>> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
>>> @@ -323,6 +323,7 @@ static void show_run_ticks(struct drm_printer *p, 
>>> struct drm_file *file)
>>>      struct xe_exec_queue *q;
>>>      u64 gpu_timestamp;
>>>      unsigned int fw_ref;
>>> +    int err;
>>>
>>>      /*
>>>       * Wait for any exec queue going away: their cycles will get 
>>> updated on
>>> @@ -350,7 +351,7 @@ static void show_run_ticks(struct drm_printer *p, 
>>> struct drm_file *file)
>>>      }
>>>      mutex_unlock(&xef->exec_queue.lock);
>>>
>>> -    gpu_timestamp = xe_hw_engine_read_timestamp(hwe);
>>> +    err = xe_hw_engine_read_timestamp(hwe, &gpu_timestamp);
>>
>> can't we just check for !IS_SRIOV_VF here?
>>
>>>
>>>      xe_force_wake_put(gt_to_fw(hwe->gt), fw_ref);
>>>      xe_pm_runtime_put(xe);
>>> @@ -371,8 +372,10 @@ static void show_run_ticks(struct drm_printer 
>>> *p, struct drm_file *file)
>>>          class_name = xe_hw_engine_class_to_str(class);
>>>          drm_printf(p, "drm-cycles-%s:\t%llu\n",
>>>                 class_name, xef->run_ticks[class]);
>>> -        drm_printf(p, "drm-total-cycles-%s:\t%llu\n",
>>> -               class_name, gpu_timestamp);
>>> +
>>> +        if (!err)
>>> +            drm_printf(p, "drm-total-cycles-%s:\t%llu\n",
>>
>> are we sure we don't break any tools that might look for this?
> 
> It may break if the tool is expecting drm-total-cycles-* to exist when it
> reads a drm-cycles-*. Also there isn't much value afaik from showing just
> the total cycles. IMO we could just skip the entire function
> on IS_SRIOV_VF().

igt@xe_drm_fdinfo is passing, as its checks are immune to the lack of 
drm-total-cycles-*. However, the only information we get is that the 
engine was busy (and comparison with spin->timestamp passes), with no 
utilization visible in gputop (the tool does not break).

We could "fool" the tools by setting drm-total-cycles to a CPU-based 
timestamp:

drm-total-cycles = mul_u64_u32_div(local_clock(),
                                    hwe->gt->info.reference_clock,
                                    NSEC_PER_SEC);

However, this is a no-go, as it could introduce more problems than benefits.

I also checked qmassa, but it does not handle VF devices (for unrelated 
reasons).

I'll send a second version skipping show_run_ticks if VF.

Thanks,
marcin
> 
> Lucas De Marchi
> 
>>
>>> +                   class_name, gpu_timestamp);
>>>
>>>          if (capacity[class] > 1)
>>>              drm_printf(p, "drm-engine-capacity-%s:\t%lu\n",
>>