[Mesa-dev] [PATCH v2 1/1] clover: Wait for requested operation if blocking flag is set

[Francisco Jerez]

These changes are somewhat redundant and potentially
performance-impacting, the reason is that in the OpenCL API,
clEnqueueWrite* commands are specified to block until the memory
provided by the application as origin can be reused safely (i.e. until
soft_copy_op()() runs), not necessarily until the transfer to graphics
memory has completed (which is what hard_event::wait() will wait for).
OTOH reads and maps as implemented by soft_copy_op and friends are
essentially blocking so the wait() call is redundant in most cases.

The only reason why it might be useful to behave differently on blocking
transfers is that the application may have specified a user event in the
event dependency list, which may cause the soft_copy_op() call to be
delayed until the application signals the user event.  In order to fix
that it should really only be necessary to wait for the event action to
be executed, not necessarily its associated GPU work.

Last time this issue came up (yeah it's not the first time) I proposed
the patches below to add a mechanism to wait for the event action only,
feel free to include it as PATCH 0.1 and 0.2 of this series (it's been a
while so they may no longer apply cleanly).

Thank you.

Jan Vesely <jan.vesely at rutgers.edu> writes:

> v2: wait in map_buffer and map_image as well
>
> Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> ---
> Hi Aaron,
>
> yes, I think you're right, we should wait in Map* as well.
> If nothing else it's consistent, even if passing the flag to add_map might make it unnecessary (haven't actually checked).
>
> thanks,
> Jan
>
>  src/gallium/state_trackers/clover/api/transfer.cpp | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/transfer.cpp b/src/gallium/state_trackers/clover/api/transfer.cpp
> index f7046253be..729a34590e 100644
> --- a/src/gallium/state_trackers/clover/api/transfer.cpp
> +++ b/src/gallium/state_trackers/clover/api/transfer.cpp
> @@ -295,6 +295,9 @@ clEnqueueReadBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     &mem, obj_origin, obj_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -325,6 +328,9 @@ clEnqueueWriteBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     ptr, {}, obj_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -362,6 +368,9 @@ clEnqueueReadBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     &mem, obj_origin, obj_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -399,6 +408,9 @@ clEnqueueWriteBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     ptr, host_origin, host_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -504,6 +516,9 @@ clEnqueueReadImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     &img, src_origin, src_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -538,6 +553,9 @@ clEnqueueWriteImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>                     ptr, {}, src_pitch,
>                     region));
>  
> +   if (blocking)
> +       hev().wait();
> +
>     ret_object(rd_ev, hev);
>     return CL_SUCCESS;
>  
> @@ -667,7 +685,11 @@ clEnqueueMapBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>  
>     void *map = mem.resource(q).add_map(q, flags, blocking, obj_origin, region);
>  
> -   ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_BUFFER, deps));
> +   auto hev = create<hard_event>(q, CL_COMMAND_MAP_BUFFER, deps);
> +   if (blocking)
> +       hev().wait();
> +
> +   ret_object(rd_ev, hev);
>     ret_error(r_errcode, CL_SUCCESS);
>     return map;
>  
> @@ -695,7 +717,11 @@ clEnqueueMapImage(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking,
>  
>     void *map = img.resource(q).add_map(q, flags, blocking, origin, region);
>  
> -   ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_IMAGE, deps));
> +   auto hev = create<hard_event>(q, CL_COMMAND_MAP_IMAGE, deps);
> +   if (blocking)
> +       hev().wait();
> +
> +   ret_object(rd_ev, hev);
>     ret_error(r_errcode, CL_SUCCESS);
>     return map;
>  
> -- 
> 2.13.3

-------------- 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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170802/0f3158ff/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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170802/0f3158ff/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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170802/0f3158ff/attachment.sig>