| Message ID | 20200715115147.11866-18-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/66] drm/i915: Reduce i915_request.lock contention for i915_request_wait | expand |
On 7/15/20 1:50 PM, Chris Wilson wrote: > Currently, if an error is raised we always call the cleanup locally > [and skip the main work callback]. However, some future users Could you add an example of those future users? > may need > to take a mutex to cleanup and so we cannot immediately execute the > cleanup as we may still be in interrupt context. > > With the execute-immediate flag, for most cases this should result in > immediate cleanup of an error. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Otherwise Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>
Quoting Thomas Hellström (Intel) (2020-07-31 10:03:59) > > On 7/15/20 1:50 PM, Chris Wilson wrote: > > Currently, if an error is raised we always call the cleanup locally > > [and skip the main work callback]. However, some future users > Could you add an example of those future users? In the next (or two) patch, the code needs to do the error cleanup from process context. Since the error paths should be relatively infrequent, and more often than not raised synchronously, I didn't see a reason to build in a flag to say whether or not the release-on-error could be performed immediately from the interrupt context. The example in this series is that even if an error is thrown, we have committed changes to the ppGTT layout (in particular marking PTE to be evicted) and so we must complete unbinding the old pages from the ppGTT, otherwise they may remain accessible. -Chris
On 7/31/20 3:28 PM, Chris Wilson wrote: > Quoting Thomas Hellström (Intel) (2020-07-31 10:03:59) >> On 7/15/20 1:50 PM, Chris Wilson wrote: >>> Currently, if an error is raised we always call the cleanup locally >>> [and skip the main work callback]. However, some future users >> Could you add an example of those future users? > In the next (or two) patch, the code needs to do the error cleanup from > process context. Since the error paths should be relatively infrequent, > and more often than not raised synchronously, I didn't see a reason to > build in a flag to say whether or not the release-on-error could be > performed immediately from the interrupt context. > > The example in this series is that even if an error is thrown, we have > committed changes to the ppGTT layout (in particular marking PTE to be > evicted) and so we must complete unbinding the old pages from the ppGTT, > otherwise they may remain accessible. Thanks. > I was mostly thinking if this or something similar could be added to the commit message to aid in understanding why the change is needed. /Thomas > -Chris
diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.c b/drivers/gpu/drm/i915/i915_sw_fence_work.c index a3a81bb8f2c3..29f63ebc24e8 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence_work.c +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.c @@ -16,11 +16,14 @@ static void fence_complete(struct dma_fence_work *f) static void fence_work(struct work_struct *work) { struct dma_fence_work *f = container_of(work, typeof(*f), work); - int err; - err = f->ops->work(f); - if (err) - dma_fence_set_error(&f->dma, err); + if (!f->dma.error) { + int err; + + err = f->ops->work(f); + if (err) + dma_fence_set_error(&f->dma, err); + } fence_complete(f); dma_fence_put(&f->dma); @@ -36,15 +39,11 @@ fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state) if (fence->error) dma_fence_set_error(&f->dma, fence->error); - if (!f->dma.error) { - dma_fence_get(&f->dma); - if (test_bit(DMA_FENCE_WORK_IMM, &f->dma.flags)) - fence_work(&f->work); - else - queue_work(system_unbound_wq, &f->work); - } else { - fence_complete(f); - } + dma_fence_get(&f->dma); + if (test_bit(DMA_FENCE_WORK_IMM, &f->dma.flags)) + fence_work(&f->work); + else + queue_work(system_unbound_wq, &f->work); break; case FENCE_FREE:
Currently, if an error is raised we always call the cleanup locally [and skip the main work callback]. However, some future users may need to take a mutex to cleanup and so we cannot immediately execute the cleanup as we may still be in interrupt context. With the execute-immediate flag, for most cases this should result in immediate cleanup of an error. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_sw_fence_work.c | 25 +++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-)