| Message ID | 20160829181613.30722-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris, 2016-08-29 Chris Wilson <chris@chris-wilson.co.uk>: > If we being polled with a timeout of zero, a nonblocking busy query, > we don't need to install any fence callbacks as we will not be waiting. > As we only install the callback once, the overhead comes from the atomic > bit test that also causes serialisation between threads. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: linux-media@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linaro-mm-sig@lists.linaro.org > --- > drivers/dma-buf/sync_file.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Indeed, we can shortcut this. Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Gustavo
Hi Chris, On 29 August 2016 at 23:56, Gustavo Padovan <gustavo@padovan.org> wrote: > Hi Chris, > > 2016-08-29 Chris Wilson <chris@chris-wilson.co.uk>: > >> If we being polled with a timeout of zero, a nonblocking busy query, >> we don't need to install any fence callbacks as we will not be waiting. >> As we only install the callback once, the overhead comes from the atomic >> bit test that also causes serialisation between threads. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Sumit Semwal <sumit.semwal@linaro.org> >> Cc: Gustavo Padovan <gustavo@padovan.org> >> Cc: linux-media@vger.kernel.org >> Cc: dri-devel@lists.freedesktop.org >> Cc: linaro-mm-sig@lists.linaro.org >> --- >> drivers/dma-buf/sync_file.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Indeed, we can shortcut this. > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Gustavo Thanks; pushed to drm-misc. Best, Sumit.
Hi Chris and Gustavo, On Mon, Aug 29, 2016 at 07:16:13PM +0100, Chris Wilson wrote: > If we being polled with a timeout of zero, a nonblocking busy query, > we don't need to install any fence callbacks as we will not be waiting. > As we only install the callback once, the overhead comes from the atomic > bit test that also causes serialisation between threads. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: linux-media@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linaro-mm-sig@lists.linaro.org > --- > drivers/dma-buf/sync_file.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 486d29c1a830..abb5fdab75fd 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -306,7 +306,8 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) > > poll_wait(file, &sync_file->wq, wait); > > - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { > + if (!poll_does_not_wait(wait) && > + !test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { > if (fence_add_callback(sync_file->fence, &sync_file->cb, > fence_check_cb_func) < 0) > wake_up_all(&sync_file->wq); This commit is causing an error on one of the tests that Robert Foss submitted for i-g-t. The one that does random merge of fences from different timelines. A simple version of the test that still triggers this is: static void test_sync_simple_merge(void) { int fence1, fence2, fence_merge, timeline1, timeline2; int ret; timeline1 = sw_sync_timeline_create(); timeline2 = sw_sync_timeline_create(); fence1 = sw_sync_fence_create(timeline1, 1); fence2 = sw_sync_fence_create(timeline2, 2); fence_merge = sw_sync_merge(fence1, fence2); sw_sync_timeline_inc(timeline1, 5); sw_sync_timeline_inc(timeline2, 5); ret = sw_sync_wait(fence_merge, 0); igt_assert_f(ret > 0, "Failure triggering fence\n"); sw_sync_fence_destroy(fence_merge); sw_sync_fence_destroy(fence1); sw_sync_fence_destroy(fence2); sw_sync_timeline_destroy(timeline1); sw_sync_timeline_destroy(timeline2); } It looks like you cannot trust fence_is_signaled() without a fence_add_callback(). I think the fence_array->num_pending won't get updated. Although I couldn't figure out why it only happens if you merge fences from different timelines. Regards, Rafael
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 486d29c1a830..abb5fdab75fd 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -306,7 +306,8 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) poll_wait(file, &sync_file->wq, wait); - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { + if (!poll_does_not_wait(wait) && + !test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { if (fence_add_callback(sync_file->fence, &sync_file->cb, fence_check_cb_func) < 0) wake_up_all(&sync_file->wq);
If we being polled with a timeout of zero, a nonblocking busy query, we don't need to install any fence callbacks as we will not be waiting. As we only install the callback once, the overhead comes from the atomic bit test that also causes serialisation between threads. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Gustavo Padovan <gustavo@padovan.org> Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/dma-buf/sync_file.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)