Re: [PATCH] drm/xe/client: Check return value of xe_force_wake_get

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

Hi Lucas,

On 5/29/2024 6:24 PM, Lucas De Marchi wrote:
> On Wed, May 29, 2024 at 10:50:34AM GMT, Nirmoy Das wrote:
>> Hi Rodrigo,
>>
>> On 5/28/2024 7:22 PM, Rodrigo Vivi wrote:
>>> On Tue, May 28, 2024 at 12:11:32PM +0200, Nirmoy Das wrote:
>>>> xe_force_wake_get() can return error so check it's return value
>>>> before reading gpu_timestamp value.
>>>>
>>>> Fixes: 188ced1e0ff8 ("drm/xe/client: Print runtime to fdinfo")
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/xe/xe_drm_client.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c 
>>>> b/drivers/gpu/drm/xe/xe_drm_client.c
>>>> index 4a19b771e3a0..0c13395ffd34 100644
>>>> --- a/drivers/gpu/drm/xe/xe_drm_client.c
>>>> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
>>>> @@ -245,7 +245,7 @@ static void show_run_ticks(struct drm_printer 
>>>> *p, struct drm_file *file)
>>>>      struct xe_gt *gt;
>>>>      struct xe_hw_engine *hwe;
>>>>      struct xe_exec_queue *q;
>>>> -    u64 gpu_timestamp;
>>>> +    u64 gpu_timestamp = 0;
>>>>      xe_pm_runtime_get(xe);
>>>> @@ -264,7 +264,9 @@ static void show_run_ticks(struct drm_printer 
>>>> *p, struct drm_file *file)
>>>>          if (!hwe)
>>>>              continue;
>>>> -        xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>>>> +        if (xe_force_wake_get(gt_to_fw(gt), XE_FW_GT))
>>>> +            break;
>>> worth adding a debug/err msg?
>>
>> xe_force_wake_get() will throw a drm_notice on error. May be we 
>> should add a __builtin_return_address() in xe_force_wake_get() to 
>> know the caller as
>>
>> the current pattern seems to be to just return early on error ?
>
> humn... but just continuing here and returning something with a gpu
> timestamp == 0 doesn't seem something good. I think we should set hwe to
> NULL, so the function doesn't continue and just doesn't print anything
> rather than printing wrong data.


Yes, make sense to me. I will resend with setting hwe to NULL

Thanks,

Nirmoy

>
> Lucas De Marchi
>
>>
>>>
>>> but up to you:
>>>
>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>>
>>>> +
>>>>          gpu_timestamp = xe_hw_engine_read_timestamp(hwe);
>>>>          xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>>>>          break;
>>>> -- 
>>>> 2.42.0
>>>>