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@xxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxxx>
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
}
