| Message ID | 20180629225419.5832-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/06/2018 23:54, Chris Wilson wrote: > Currently all callers are responsible for adding the vma to the active > timeline and then exporting its fence. Combine the two operations into > i915_vma_move_to_active() to move all the extra handling from the > callers to the single site. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 47 +++++++++---------- > drivers/gpu/drm/i915/selftests/huge_pages.c | 4 -- > .../drm/i915/selftests/i915_gem_coherency.c | 4 -- > .../gpu/drm/i915/selftests/i915_gem_context.c | 4 -- > .../gpu/drm/i915/selftests/i915_gem_object.c | 4 -- > 5 files changed, 21 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index c2dd9b4cdace..91f20445147f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1166,15 +1166,9 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb, > > GEM_BUG_ON(!reservation_object_test_signaled_rcu(batch->resv, true)); > i915_vma_move_to_active(batch, rq, 0); > - reservation_object_lock(batch->resv, NULL); > - reservation_object_add_excl_fence(batch->resv, &rq->fence); > - reservation_object_unlock(batch->resv); > i915_vma_unpin(batch); > > i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); > - reservation_object_lock(vma->resv, NULL); > - reservation_object_add_excl_fence(vma->resv, &rq->fence); > - reservation_object_unlock(vma->resv); > > rq->batch = batch; > > @@ -1771,25 +1765,6 @@ static int eb_relocate(struct i915_execbuffer *eb) > return eb_relocate_slow(eb); > } > > -static void eb_export_fence(struct i915_vma *vma, > - struct i915_request *rq, > - unsigned int flags) > -{ > - struct reservation_object *resv = vma->resv; > - > - /* > - * Ignore errors from failing to allocate the new fence, we can't > - * handle an error right now. Worst case should be missed > - * synchronisation leading to rendering corruption. > - */ > - reservation_object_lock(resv, NULL); > - if (flags & EXEC_OBJECT_WRITE) > - reservation_object_add_excl_fence(resv, &rq->fence); > - else if (reservation_object_reserve_shared(resv) == 0) > - reservation_object_add_shared_fence(resv, &rq->fence); > - reservation_object_unlock(resv); > -} > - > static int eb_move_to_gpu(struct i915_execbuffer *eb) > { > const unsigned int count = eb->buffer_count; > @@ -1844,7 +1819,6 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb) > struct i915_vma *vma = eb->vma[i]; > > i915_vma_move_to_active(vma, eb->request, flags); > - eb_export_fence(vma, eb->request, flags); > > __eb_unreserve_vma(vma, flags); > vma->exec_flags = NULL; > @@ -1884,6 +1858,25 @@ static bool i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec) > return true; > } > > +static void export_fence(struct i915_vma *vma, > + struct i915_request *rq, > + unsigned int flags) > +{ > + struct reservation_object *resv = vma->resv; > + > + /* > + * Ignore errors from failing to allocate the new fence, we can't > + * handle an error right now. Worst case should be missed > + * synchronisation leading to rendering corruption. > + */ > + reservation_object_lock(resv, NULL); > + if (flags & EXEC_OBJECT_WRITE) > + reservation_object_add_excl_fence(resv, &rq->fence); > + else if (reservation_object_reserve_shared(resv) == 0) > + reservation_object_add_shared_fence(resv, &rq->fence); > + reservation_object_unlock(resv); > +} > + > void i915_vma_move_to_active(struct i915_vma *vma, > struct i915_request *rq, > unsigned int flags) > @@ -1921,6 +1914,8 @@ void i915_vma_move_to_active(struct i915_vma *vma, > > if (flags & EXEC_OBJECT_NEEDS_FENCE) > i915_gem_active_set(&vma->last_fence, rq); > + > + export_fence(vma, rq, flags); Don't care about new extra reservation object operations from the other i915_move_to_active_callers? Regards, Tvrtko > } > > static int i915_reset_gen7_sol_offsets(struct i915_request *rq) > diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c > index b5e87fcdcdae..358fc81f6c99 100644 > --- a/drivers/gpu/drm/i915/selftests/huge_pages.c > +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c > @@ -998,10 +998,6 @@ static int gpu_write(struct i915_vma *vma, > > i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); > > - reservation_object_lock(vma->resv, NULL); > - reservation_object_add_excl_fence(vma->resv, &rq->fence); > - reservation_object_unlock(vma->resv); > - > err_request: > i915_request_add(rq); > > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c > index a4900091ae3d..11427aae0853 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c > @@ -225,10 +225,6 @@ static int gpu_set(struct drm_i915_gem_object *obj, > i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); > i915_vma_unpin(vma); > > - reservation_object_lock(obj->resv, NULL); > - reservation_object_add_excl_fence(obj->resv, &rq->fence); > - reservation_object_unlock(obj->resv); > - > i915_request_add(rq); > > return 0; > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c > index cc848ceeb3c3..81ed87aa0a4d 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c > @@ -178,10 +178,6 @@ static int gpu_fill(struct drm_i915_gem_object *obj, > i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); > i915_vma_unpin(vma); > > - reservation_object_lock(obj->resv, NULL); > - reservation_object_add_excl_fence(obj->resv, &rq->fence); > - reservation_object_unlock(obj->resv); > - > i915_request_add(rq); > > return 0; > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_object.c b/drivers/gpu/drm/i915/selftests/i915_gem_object.c > index 77dd7a510ea6..fa5a0654314a 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_object.c > @@ -456,10 +456,6 @@ static int make_obj_busy(struct drm_i915_gem_object *obj) > > i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); > > - reservation_object_lock(vma->resv, NULL); > - reservation_object_add_excl_fence(vma->resv, &rq->fence); > - reservation_object_unlock(vma->resv); > - > i915_request_add(rq); > > i915_gem_object_set_active_reference(obj); >
Quoting Tvrtko Ursulin (2018-07-02 12:34:51) > > @@ -1921,6 +1914,8 @@ void i915_vma_move_to_active(struct i915_vma *vma, > > > > if (flags & EXEC_OBJECT_NEEDS_FENCE) > > i915_gem_active_set(&vma->last_fence, rq); > > + > > + export_fence(vma, rq, flags); > > Don't care about new extra reservation object operations from the other > i915_move_to_active_callers? No. Not filling the reservation_object is an underhand shortcut. It should never be not correct to track the fences for an object. Doing it in one place correctly, should prevent cheating. -Chris
On 02/07/2018 12:44, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-07-02 12:34:51) >>> @@ -1921,6 +1914,8 @@ void i915_vma_move_to_active(struct i915_vma *vma, >>> >>> if (flags & EXEC_OBJECT_NEEDS_FENCE) >>> i915_gem_active_set(&vma->last_fence, rq); >>> + >>> + export_fence(vma, rq, flags); >> >> Don't care about new extra reservation object operations from the other >> i915_move_to_active_callers? > > No. Not filling the reservation_object is an underhand shortcut. It > should never be not correct to track the fences for an object. Doing it > in one place correctly, should prevent cheating. Fair enough. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c2dd9b4cdace..91f20445147f 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1166,15 +1166,9 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb, GEM_BUG_ON(!reservation_object_test_signaled_rcu(batch->resv, true)); i915_vma_move_to_active(batch, rq, 0); - reservation_object_lock(batch->resv, NULL); - reservation_object_add_excl_fence(batch->resv, &rq->fence); - reservation_object_unlock(batch->resv); i915_vma_unpin(batch); i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); - reservation_object_lock(vma->resv, NULL); - reservation_object_add_excl_fence(vma->resv, &rq->fence); - reservation_object_unlock(vma->resv); rq->batch = batch; @@ -1771,25 +1765,6 @@ static int eb_relocate(struct i915_execbuffer *eb) return eb_relocate_slow(eb); } -static void eb_export_fence(struct i915_vma *vma, - struct i915_request *rq, - unsigned int flags) -{ - struct reservation_object *resv = vma->resv; - - /* - * Ignore errors from failing to allocate the new fence, we can't - * handle an error right now. Worst case should be missed - * synchronisation leading to rendering corruption. - */ - reservation_object_lock(resv, NULL); - if (flags & EXEC_OBJECT_WRITE) - reservation_object_add_excl_fence(resv, &rq->fence); - else if (reservation_object_reserve_shared(resv) == 0) - reservation_object_add_shared_fence(resv, &rq->fence); - reservation_object_unlock(resv); -} - static int eb_move_to_gpu(struct i915_execbuffer *eb) { const unsigned int count = eb->buffer_count; @@ -1844,7 +1819,6 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb) struct i915_vma *vma = eb->vma[i]; i915_vma_move_to_active(vma, eb->request, flags); - eb_export_fence(vma, eb->request, flags); __eb_unreserve_vma(vma, flags); vma->exec_flags = NULL; @@ -1884,6 +1858,25 @@ static bool i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec) return true; } +static void export_fence(struct i915_vma *vma, + struct i915_request *rq, + unsigned int flags) +{ + struct reservation_object *resv = vma->resv; + + /* + * Ignore errors from failing to allocate the new fence, we can't + * handle an error right now. Worst case should be missed + * synchronisation leading to rendering corruption. + */ + reservation_object_lock(resv, NULL); + if (flags & EXEC_OBJECT_WRITE) + reservation_object_add_excl_fence(resv, &rq->fence); + else if (reservation_object_reserve_shared(resv) == 0) + reservation_object_add_shared_fence(resv, &rq->fence); + reservation_object_unlock(resv); +} + void i915_vma_move_to_active(struct i915_vma *vma, struct i915_request *rq, unsigned int flags) @@ -1921,6 +1914,8 @@ void i915_vma_move_to_active(struct i915_vma *vma, if (flags & EXEC_OBJECT_NEEDS_FENCE) i915_gem_active_set(&vma->last_fence, rq); + + export_fence(vma, rq, flags); } static int i915_reset_gen7_sol_offsets(struct i915_request *rq) diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c index b5e87fcdcdae..358fc81f6c99 100644 --- a/drivers/gpu/drm/i915/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c @@ -998,10 +998,6 @@ static int gpu_write(struct i915_vma *vma, i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); - reservation_object_lock(vma->resv, NULL); - reservation_object_add_excl_fence(vma->resv, &rq->fence); - reservation_object_unlock(vma->resv); - err_request: i915_request_add(rq); diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c index a4900091ae3d..11427aae0853 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c @@ -225,10 +225,6 @@ static int gpu_set(struct drm_i915_gem_object *obj, i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); i915_vma_unpin(vma); - reservation_object_lock(obj->resv, NULL); - reservation_object_add_excl_fence(obj->resv, &rq->fence); - reservation_object_unlock(obj->resv); - i915_request_add(rq); return 0; diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c index cc848ceeb3c3..81ed87aa0a4d 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c @@ -178,10 +178,6 @@ static int gpu_fill(struct drm_i915_gem_object *obj, i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); i915_vma_unpin(vma); - reservation_object_lock(obj->resv, NULL); - reservation_object_add_excl_fence(obj->resv, &rq->fence); - reservation_object_unlock(obj->resv); - i915_request_add(rq); return 0; diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_object.c b/drivers/gpu/drm/i915/selftests/i915_gem_object.c index 77dd7a510ea6..fa5a0654314a 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_object.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_object.c @@ -456,10 +456,6 @@ static int make_obj_busy(struct drm_i915_gem_object *obj) i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE); - reservation_object_lock(vma->resv, NULL); - reservation_object_add_excl_fence(vma->resv, &rq->fence); - reservation_object_unlock(vma->resv); - i915_request_add(rq); i915_gem_object_set_active_reference(obj);
Currently all callers are responsible for adding the vma to the active timeline and then exporting its fence. Combine the two operations into i915_vma_move_to_active() to move all the extra handling from the callers to the single site. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 47 +++++++++---------- drivers/gpu/drm/i915/selftests/huge_pages.c | 4 -- .../drm/i915/selftests/i915_gem_coherency.c | 4 -- .../gpu/drm/i915/selftests/i915_gem_context.c | 4 -- .../gpu/drm/i915/selftests/i915_gem_object.c | 4 -- 5 files changed, 21 insertions(+), 42 deletions(-)