Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 21.11.2022 11:59, Janusz Krzysztofik wrote:
> On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
>> Hi Andrzej,
>>
>> Thanks for your comment.
>>
>> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
>>>
>>> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
>>>> Hi Nimroy,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>>>>> On 11/18/2022 11:42 AM, Janusz Krzysztofik wrote:
>>>>>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
>>>>>> success.  However, we have no protection from passing back 0 potentially
>>>>>> returned by a call to dma_fence_wait_timeout() when it succedes right
>>>>>> after its timeout has expired.
>>>>>>
>>>>>> Replace 0 with -ETIME before potentially using the timeout value as return
>>>>>> code, so -ETIME is returned if there are still some requests not retired
>>>>>> after timeout, 0 otherwise.
>>>>>>
>>>>>> v2: Move the added lines down so flush_submission() is not affected.
>>>>>>
>>>>>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with
>>>> retire_request")
>>>>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>>>>> Cc: stable@vger.kernel.org # v5.5+
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
>>>> drm/i915/gt/intel_gt_requests.c
>>>>>> index edb881d756309..3ac4603eeb4ee 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>>> @@ -199,6 +199,9 @@ out_active:	spin_lock(&timelines->lock);
>>>>>>     	if (remaining_timeout)
>>>>>>     		*remaining_timeout = timeout;
>>>>>>     
>>>>>> +	if (!timeout)
>>>>>> +		timeout = -ETIME;
>>>>> This will return error, -ETIME when 0 timeout is passed,
>>>>> intel_gt_retire_requests().
>>>> Yes, but only when active_count is not 0 after we loop through
>>>> timelines->active_list calling retire_requests() on each and counting up
>>>> failures in active_count.
>>>
>>> Moving this line just after the call to dma_fence_wait_timeout should
>>> solve the controversy.
>>
>> But that would break our need to pass 0, not -ETIME, to flush_submission() in
>> case the initial value of timeout was 0, as pointed out by Chris during our
>> discussion on v2.
>>
>> Maybe an inline comment above the added lines that explains why we are doing
>> this could help?
> 
> How about not adding those two lines but modifying the return line instead?
> 
> -	return active_count ? timeout : 0;
> +	return active_count ? timeout ?: -ETIME : 0;

Personally I would translate ret value from dma_fence* API ASAP, and 
call flush_submission conditionally - to limit coexistence of both APIs.
But this looks correct to me, as well.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> 
> Would that be self explanatory?
> 
> Thanks,
> Janusz
> 
>>
>> Thanks,
>> Janusz
>>
>>>
>>> Regards
>>> Andrzej
>>>
>>>>
>>>>> We don't want that.
>>>> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
>>>> to return 0 unconditionally, or are we rather interested if those calls to
>>>> retire_requests() succeeded?
>>>>
>>>>> I think you can use a separate variable to store
>>>>> return val from the dma_fence_wait_timeout()
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Nirmoy
>>>>>
>>>>>> +
>>>>>>     	return active_count ? timeout : 0;
>>>> If active count is 0, we return 0 regardless of timeout value, and that's OK.
>>>> However, if active_count is not 0, we shouldn't return 0, I believe, we should
>>>> return either remaining time if some left, or error (-ETIME) if not.  If you
>>>> think I'm wrong, please explain why.
>>>>
>>>> Thanks,
>>>> Janusz
>>>>
>>>>>>     }
>>>>>>     
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 
> 
>