[Mesa-dev] [PATCH] clover: Fix bug with computing hard_event status

[Francisco Jerez]

Tom Stellard <thomas.stellard at amd.com> writes:

> pipe_context::flush() can return a NULL fence if the queue is already
> empty, so we should not assume that an event with a NULL fence
> has the status of CL_QUEUED.
>

This seems suspicious...  On the one hand it doesn't seem to be a
documented "feature" of pipe_context::flush to return NULL except in
error conditions (I'm pretty sure other drivers like nouveau won't), and
it seems like it could easily break assumptions of other state trackers.

IMO pipe_context::flush() should respect the invariant that whatever is
returned in the fence output argument (unless some error occurred) be a
valid argument for pipe_screen::fence_finish() and ::fence_signalled()
-- I don't think NULL is?

On the other hand this leaves me wondering how could the queue already
be empty when clover calls pipe_context::flush() -- I assume by queue
you mean the pipe driver's?  The fact that clover calls
pipe_context::flush() implies that clover's event queue is not empty
(i.e. there have been commands enqueued to the pipe driver since the
last call to pipe_context::flush()).  It sounds like this mismatch
between clover's and the pipe driver's command queue might be caused by
some race condition elsewhere?

Thanks.

> CC: 10.6 <mesa-stable at lists.freedesktop.org>
> ---
>  src/gallium/state_trackers/clover/core/event.cpp | 7 ++++---
>  src/gallium/state_trackers/clover/core/event.hpp | 1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/event.cpp b/src/gallium/state_trackers/clover/core/event.cpp
> index d75b839..b973c78 100644
> --- a/src/gallium/state_trackers/clover/core/event.cpp
> +++ b/src/gallium/state_trackers/clover/core/event.cpp
> @@ -118,7 +118,7 @@ event::wait() const {
>  hard_event::hard_event(command_queue &q, cl_command_type command,
>                         const ref_vector<event> &deps, action action) :
>     event(q.context(), deps, profile(q, action), [](event &ev){}),
> -   _queue(q), _command(command), _fence(NULL) {
> +   _queue(q), _command(command), _fence(NULL), _fenced(false) {
>     if (q.profiling_enabled())
>        _time_queued = timestamp::current(q);
>  
> @@ -138,7 +138,7 @@ hard_event::status() const {
>     if (event::status() < 0)
>        return event::status();
>  
> -   else if (!_fence)
> +   else if (!_fenced)
>        return CL_QUEUED;
>  
>     else if (!screen->fence_finish(screen, _fence, 0))
> @@ -167,7 +167,7 @@ hard_event::wait() const {
>     if (status() == CL_QUEUED)
>        queue()->flush();
>  
> -   if (!_fence ||
> +   if (!_fenced ||
>         !screen->fence_finish(screen, _fence, PIPE_TIMEOUT_INFINITE))
>        throw error(CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
>  }
> @@ -196,6 +196,7 @@ void
>  hard_event::fence(pipe_fence_handle *fence) {
>     pipe_screen *screen = queue()->device().pipe;
>     screen->fence_reference(screen, &_fence, fence);
> +   _fenced = true;
>  }
>  
>  event::action
> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
> index 6469e48..fac62d2 100644
> --- a/src/gallium/state_trackers/clover/core/event.hpp
> +++ b/src/gallium/state_trackers/clover/core/event.hpp
> @@ -137,6 +137,7 @@ namespace clover {
>        const intrusive_ref<command_queue> _queue;
>        cl_command_type _command;
>        pipe_fence_handle *_fence;
> +      bool _fenced;
>        lazy<cl_ulong> _time_queued, _time_submit, _time_start, _time_end;
>     };
>  
> -- 
> 2.0.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150711/8f172391/attachment.sig>