Message ID | 20180307171303.29466-1-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Quoting Chris Wilson (2018-03-07 17:13:03) > Currently, we only allow ourselves to prune the fences so long as > all the waits completed (i.e. all the fences we checked were signaled), > and that the reservation snapshot did not change across the wait. > However, if we only waited for a subset of the reservation object, i.e. > just waiting for the last writer to complete as opposed to all readers > as well, then we would erroneously conclude we could prune the fences as > indeed although all of our waits were successful, they did not represent > the totality of the reservation object. > > v2: We only need to check the shared fences due to construction (i.e. > all of the shared fences will be later than the exclusive fence, if > any). > Testcase: igt/drv_hangman As it turns out, that is pretty much the only way we can observe this bug. (At least not without drinking more than one cup of tea.) You could construct something to try and overwrite the reader on another engine, tricky to observe though. However, it is the goal of selftests/live_coherency and gem_exec_flush to try and catch this type of error, so should try harder. -Chris
On 7 March 2018 at 17:13, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Currently, we only allow ourselves to prune the fences so long as > all the waits completed (i.e. all the fences we checked were signaled), > and that the reservation snapshot did not change across the wait. > However, if we only waited for a subset of the reservation object, i.e. > just waiting for the last writer to complete as opposed to all readers > as well, then we would erroneously conclude we could prune the fences as > indeed although all of our waits were successful, they did not represent > the totality of the reservation object. > > v2: We only need to check the shared fences due to construction (i.e. > all of the shared fences will be later than the exclusive fence, if > any). > > Fixes: e54ca9774777 ("drm/i915: Remove completed fences after a wait") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index a5bd07338b46..9b48b5101357 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -433,20 +433,28 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, > dma_fence_put(shared[i]); > kfree(shared); > > + /* > + * If both shared fences and an exclusive fence exist, > + * then by construction the shared fences must be later > + * than the exclusive fence. If we successfully wait for > + * all the shared fences, we know that the exclusive fence > + * must all be signaled. If all the shared fences are > + * signaled, we can prune the array and recover the > + * floating references on the fences/requests. > + */ > prune_fences = count && timeout >= 0; > } else { > excl = reservation_object_get_excl_rcu(resv); > } > > - if (excl && timeout >= 0) { > + if (excl && timeout >= 0) > timeout = i915_gem_object_wait_fence(excl, flags, timeout, > rps_client); > - prune_fences = timeout >= 0; > - } > > dma_fence_put(excl); > > - /* Oportunistically prune the fences iff we know they have *all* been > + /* > + * Oportunistically prune the fences iff we know they have *all* been Opportunistically Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Quoting Matthew Auld (2018-03-08 17:48:48) > On 7 March 2018 at 17:13, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Currently, we only allow ourselves to prune the fences so long as > > all the waits completed (i.e. all the fences we checked were signaled), > > and that the reservation snapshot did not change across the wait. > > However, if we only waited for a subset of the reservation object, i.e. > > just waiting for the last writer to complete as opposed to all readers > > as well, then we would erroneously conclude we could prune the fences as > > indeed although all of our waits were successful, they did not represent > > the totality of the reservation object. > > > > v2: We only need to check the shared fences due to construction (i.e. > > all of the shared fences will be later than the exclusive fence, if > > any). > > > > Fixes: e54ca9774777 ("drm/i915: Remove completed fences after a wait") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Matthew Auld <matthew.auld@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index a5bd07338b46..9b48b5101357 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -433,20 +433,28 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, > > dma_fence_put(shared[i]); > > kfree(shared); > > > > + /* > > + * If both shared fences and an exclusive fence exist, > > + * then by construction the shared fences must be later > > + * than the exclusive fence. If we successfully wait for > > + * all the shared fences, we know that the exclusive fence > > + * must all be signaled. If all the shared fences are > > + * signaled, we can prune the array and recover the > > + * floating references on the fences/requests. > > + */ > > prune_fences = count && timeout >= 0; > > } else { > > excl = reservation_object_get_excl_rcu(resv); > > } > > > > - if (excl && timeout >= 0) { > > + if (excl && timeout >= 0) > > timeout = i915_gem_object_wait_fence(excl, flags, timeout, > > rps_client); > > - prune_fences = timeout >= 0; > > - } > > > > dma_fence_put(excl); > > > > - /* Oportunistically prune the fences iff we know they have *all* been > > + /* > > + * Oportunistically prune the fences iff we know they have *all* been > Opportunistically > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> And a bonus spelling fix, thank you! Pushed, -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a5bd07338b46..9b48b5101357 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -433,20 +433,28 @@ i915_gem_object_wait_reservation(struct reservation_object *resv, dma_fence_put(shared[i]); kfree(shared); + /* + * If both shared fences and an exclusive fence exist, + * then by construction the shared fences must be later + * than the exclusive fence. If we successfully wait for + * all the shared fences, we know that the exclusive fence + * must all be signaled. If all the shared fences are + * signaled, we can prune the array and recover the + * floating references on the fences/requests. + */ prune_fences = count && timeout >= 0; } else { excl = reservation_object_get_excl_rcu(resv); } - if (excl && timeout >= 0) { + if (excl && timeout >= 0) timeout = i915_gem_object_wait_fence(excl, flags, timeout, rps_client); - prune_fences = timeout >= 0; - } dma_fence_put(excl); - /* Oportunistically prune the fences iff we know they have *all* been + /* + * Oportunistically prune the fences iff we know they have *all* been * signaled and that the reservation object has not been changed (i.e. * no new fences have been added). */
Currently, we only allow ourselves to prune the fences so long as all the waits completed (i.e. all the fences we checked were signaled), and that the reservation snapshot did not change across the wait. However, if we only waited for a subset of the reservation object, i.e. just waiting for the last writer to complete as opposed to all readers as well, then we would erroneously conclude we could prune the fences as indeed although all of our waits were successful, they did not represent the totality of the reservation object. v2: We only need to check the shared fences due to construction (i.e. all of the shared fences will be later than the exclusive fence, if any). Fixes: e54ca9774777 ("drm/i915: Remove completed fences after a wait") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)