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

[Tom Stellard]

On Sat, Jul 11, 2015 at 02:35:53PM +0300, Francisco Jerez wrote:
> 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.
> 

The bug appears in programs which call clFinish() without ever
adding anything to the command queue.  In this case, radeonsi
sees that no commands have been submitted to the GPU, so it doesn't
submit the fence and sets the fence parameter to NULL.

-Tom


> > 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




> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev