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

[Francisco Jerez]

Grigori Goronzy <greg at chown.ath.cx> writes:

> 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. :)
>
So is virtually any approach to error handling :P, it's not really more
difficult to write exception-safe code than it is to write equivalent
returned-error-code-safe code, in fact it's IME less difficult as long
as you stick to RAII (which is a healthy practice on its own).

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

I think those should also call event::wait_signalled() just to make sure
that the event action has been executed already -- It may not have
executed immediately if there were any user events in the dependency
graph.

>> 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?
>
hard_event::wait() flushes the command queue, what in turn attaches a
fence to the event object marking the end of the execution of the last
batch of commands, which arguably contains whatever operations were
performed by the event action.  This assumption breaks in the case of
soft_copy_op() because it doesn't submit any actual commands to the GPU,
so calling hard_event::wait() is sub-optimal (it will wait for commands
which are completely unrelated to the copy operation), this can be fixed
by using the weaker version of wait() that doesn't care about the GPU
being already done with the work.

Thanks.

>> 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
-------------- 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/20150626/1b47f3d4/attachment.sig>