[Mesa-dev] [PATCH 1/2] clover: fix event handling of buffer operations

[Grigori Goronzy]

On 2015-06-09 22:52, Francisco Jerez wrote:
>> +
>> +   if (blocking)
>> +      hev().wait();
>> +
> 
> hard_event::wait() may fail, so this should probably be done before the
> ret_object() call to avoid leaks.

Alright... C++ exceptions are a minefield. :)

> Is there any reason you didn't make
> the same change in clEnqueueReadBuffer() and clEnqueueWriteBuffer()?
> 

Must be an oversight. I think I did that, or at least I intended to do 
so.

> Same comment as above.  Also note that this is being more strict than
> the spec requires (which I believe is what Tom was referring to).  From
> the CL 1.2 spec:
> 
> | If blocking_write is CL_TRUE, the OpenCL implementation copies the 
> data
> | referred to by ptr and enqueues the write operation in the
> | command-queue. The memory pointed to by ptr can be reused by the
> | application after the clEnqueueWriteBufferRect call returns.
> 
> The spec is giving you no guarantee that the write to the actual memory
> object will be complete by the time the clEnqueueWriteBufferRect call
> returns -- Only that your data will have been buffered somewhere and 
> the
> memory pointed to by the argument can be reused immediately by the
> application.  The reason why I was reluctant to make this change last
> time it came up was that it's likely to hurt performance unnecessarily
> because the wait() call blocks until *all* previous commands in the 
> same
> queue have completed execution, even though in the most common case the
> copy is performed synchronously using soft_copy_op(), so the wait() 
> call
> is redundant even for blocking copies.
> 

OK, maybe we could drop the wait completely for all of the "write" 
calls.

> The case with blocking reads is similar, the copy is handled
> synchronously using soft_copy_op() when no user events are present in
> the list of dependencies, so calling wait() on the event is unnecessary
> to guarantee that the execution of the read has completed, and will
> cause a pipe_context flush and wait until the most recent fence is
> signalled.
> 

I think it's reasonable to expect that the event is ready for profile 
queries after a blocking read has finished. That was the initial 
motivation for this patch. Other implementations behave like that. I 
didn't expect wait() to completely flush everything. Won't that cause a 
lot of needless flushing with event wait lists?

> Ideally we would have a weaker variant of event::wait()
> (e.g. wait_signalled()) that doesn't flush and just waits for the
> associated action call-back to have been executed without giving any
> guarantees about the corresponding GPU command.  The event interface
> doesn't expose such a functionality right now, I'm attaching two
> (completely untested) patches implementing it, you should be able to 
> use
> them as starting point to fix blocking transfers.
> 

Thanks, I'll look into that later when I get some free time.

Grigori