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

[Francisco Jerez]

Hi Grigori,

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

> Wrap MapBuffer and MapImage as hard_event actions, like other
> operations. This enables correct profiling. Also make sure to wait
> for events to finish when blocking is requested by the caller.
> ---
>  src/gallium/state_trackers/clover/api/transfer.cpp | 50 ++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/transfer.cpp b/src/gallium/state_trackers/clover/api/transfer.cpp
> index fdb9405..4986f53 100644
> --- a/src/gallium/state_trackers/clover/api/transfer.cpp
> +++ b/src/gallium/state_trackers/clover/api/transfer.cpp
> @@ -270,6 +270,18 @@ namespace {
>                                     src_obj->resource(q), src_orig);
>        };
>     }
> +
> +   ///
> +   /// Resource mapping
> +   ///
> +   template<typename T>
> +   std::function<void (event &)>
> +   map_resource_op(command_queue &q, T obj, cl_map_flags flags, bool blocking,
> +                   const vector_t &origin, const vector_t &region, void **map) {
> +      return [=, &q](event &) {
> +         *map = obj->resource(q).add_map(q, flags, blocking, origin, region);
> +      };
> +   }
>  }
>  
>  CLOVER_API cl_int
> @@ -363,6 +375,10 @@ clEnqueueReadBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     region));
>  
>     ret_object(rd_ev, hev);
> +
> +   if (blocking)
> +      hev().wait();
> +

hard_event::wait() may fail, so this should probably be done before the
ret_object() call to avoid leaks.  Is there any reason you didn't make
the same change in clEnqueueReadBuffer() and clEnqueueWriteBuffer()?

>     return CL_SUCCESS;
>  
>  } catch (error &e) {
> @@ -400,6 +416,10 @@ clEnqueueWriteBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     region));
>  
>     ret_object(rd_ev, hev);
> +
> +   if (blocking)
> +      hev().wait();
> +

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.

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.

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.

>     return CL_SUCCESS;
>  
>  } catch (error &e) {
> @@ -505,6 +525,10 @@ clEnqueueReadImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     region));
>  
>     ret_object(rd_ev, hev);
> +
> +   if (blocking)
> +      hev().wait();
> +
>     return CL_SUCCESS;
>  
>  } catch (error &e) {
> @@ -539,6 +563,10 @@ clEnqueueWriteImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     region));
>  
>     ret_object(rd_ev, hev);
> +
> +   if (blocking)
> +      hev().wait();
> +

Same comment as above for image reads and writes.

>     return CL_SUCCESS;
>  
>  } catch (error &e) {
> @@ -665,10 +693,17 @@ clEnqueueMapBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>     validate_object(q, mem, obj_origin, obj_pitch, region);
>     validate_map_flags(mem, flags);
>  
> -   void *map = mem.resource(q).add_map(q, flags, blocking, obj_origin, region);
> +   void *map = nullptr;
> +   auto hev = create<hard_event>(
> +      q, CL_COMMAND_MAP_BUFFER, deps,
> +      map_resource_op(q, &mem, flags, blocking, obj_origin, region, &map));
>  
> -   ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_BUFFER, deps));
> +   ret_object(rd_ev, hev);
>     ret_error(r_errcode, CL_SUCCESS);
> +
> +   if (blocking)
> +      hev().wait();
> +
This doesn't look right for non-blocking maps, if you wrap it in an
event you need to make sure that the action has been executed (e.g. by
using event::wait_signalled()) before you return, even if the map is
non-blocking, otherwise the "map" variable won't be initialized in time
and you'll just return nullptr.

>     return map;
>  
>  } catch (error &e) {
> @@ -693,10 +728,17 @@ clEnqueueMapImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>     validate_object(q, img, origin, region);
>     validate_map_flags(img, flags);
>  
> -   void *map = img.resource(q).add_map(q, flags, blocking, origin, region);
> +   void *map = nullptr;
> +   auto hev = create<hard_event>(
> +      q, CL_COMMAND_MAP_IMAGE, deps,
> +      map_resource_op(q, &img, flags, blocking, origin, region, &map));
>  
> -   ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_IMAGE, deps));
> +   ret_object(rd_ev, hev);
>     ret_error(r_errcode, CL_SUCCESS);
> +
> +   if (blocking)
> +      hev().wait();
> +

Same here.

>     return map;
>  
>  } catch (error &e) {
> -- 
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-clover-Wrap-event-wait_count-in-a-method-taking-care.patch
Type: text/x-diff
Size: 2811 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150609/fcff6a71/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-clover-Run-the-associated-action-before-an-event-is-.patch
Type: text/x-diff
Size: 2855 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150609/fcff6a71/attachment-0001.patch>
-------------- 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/20150609/fcff6a71/attachment.sig>