Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/11/2022 23:19, Janusz Krzysztofik wrote:
> Hi Andrzej,
> 
> Thanks for providing your R-b.
> 
> On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
>> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
>>> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
>>> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
>>> extra argument 'remaining_timeout', intended for passing back unconsumed
>>> portion of requested timeout when 0 (success) is returned.  However, when
>>> request retirement happens to succeed despite an error returned by a call
>>> to dma_fence_wait_timeout(), that error code (a negative value) is passed
>>> back instead of remaining time.  If we then pass that negative value
>>> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
>>> will be triggered.

Right, AFAICT a GEM_BUG_ON in debug builds, but in production builds 
negative timeout will get passed along all the way to schedule_timeout 
where error and call trace will be dumped. So fix appears warranted indeed.

>>> If request retirement succeeds but an error code is passed back via
>>> remaininig_timeout, we may have no clue on how much of the initial timeout
>>> might have been left for spending it on waiting for GuC to become idle.
>>> OTOH, since all pending requests have been successfully retired, that
>>> error code has been already ignored by intel_gt_retire_requests_timeout(),
>>> then we shouldn't fail.
>>>
>>> Assume no more time has been left on error and pass 0 timeout value to
>>> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
>>> already idle.
>>>
>>> v3: Don't fail on any error passed back via remaining_timeout.
>>>
>>> v2: Fix the issue on the caller side, not the provider.
>>>
>>> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> with GuC")
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Cc: stable@vger.kernel.org # v5.15+
>>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> While still open for comments from others, I'm now looking for potential
> committer.

Both patches are considered good to go?

Regards,

Tvrtko

> 
> Thanks,
> Janusz
> 
> 
>>
>> Regards
>> Andrzej
>>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
> intel_gt.c
>>> index b5ad9caa55372..7ef0edb2e37cd 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
> timeout)
>>>    			return -EINTR;
>>>    	}
>>>    
>>> -	return timeout ? timeout : intel_uc_wait_for_idle(&gt->uc,
>>> -							
> remaining_timeout);
>>> +	if (timeout)
>>> +		return timeout;
>>> +
>>> +	if (remaining_timeout < 0)
>>> +		remaining_timeout = 0;
>>> +
>>> +	return intel_uc_wait_for_idle(&gt->uc, remaining_timeout);
>>>    }
>>>    
>>>    int intel_gt_init(struct intel_gt *gt)
>>
>>
> 
> 
> 
>