[Mesa-dev] [PATCH 2/2] clover: Add clEnqueue{Marker, Barrier}WithWaitList

[EdB]

On Saturday, April 26, 2014 12:05:25 PM Francisco Jerez wrote:
> EdB <edb+mesa at sigluy.net> writes:
> 
> Looks OK to me, some nitpicks related to the coding style below.
> 
> > ---
> > 
> >  src/gallium/state_trackers/clover/api/dispatch.cpp |  4 +-
> >  src/gallium/state_trackers/clover/api/event.cpp    | 48
> >  ++++++++++++++++++---- 2 files changed, 42 insertions(+), 10
> >  deletions(-)
> > 
> > diff --git a/src/gallium/state_trackers/clover/api/dispatch.cpp
> > b/src/gallium/state_trackers/clover/api/dispatch.cpp index
> > 746372c..e4f7ea3 100644
> > --- a/src/gallium/state_trackers/clover/api/dispatch.cpp
> > +++ b/src/gallium/state_trackers/clover/api/dispatch.cpp
> > @@ -129,8 +129,8 @@ namespace clover {
> > 
> >        NULL, // clEnqueueFillBuffer
> >        NULL, // clEnqueueFillImage
> >        NULL, // clEnqueueMigrateMemObjects
> > 
> > -      NULL, // clEnqueueMarkerWithWaitList
> > -      NULL, // clEnqueueBarrierWithWaitList
> > +      clEnqueueMarkerWithWaitList,
> > +      clEnqueueBarrierWithWaitList,
> > 
> >        NULL, // clGetExtensionFunctionAddressForPlatform
> >        NULL, // clCreateFromGLTexture
> >        NULL, // clGetDeviceIDsFromD3D11KHR
> > 
> > diff --git a/src/gallium/state_trackers/clover/api/event.cpp
> > b/src/gallium/state_trackers/clover/api/event.cpp index 6b1956c..daf9577
> > 100644
> > --- a/src/gallium/state_trackers/clover/api/event.cpp
> > +++ b/src/gallium/state_trackers/clover/api/event.cpp
> > @@ -179,6 +179,27 @@ clEnqueueMarker(cl_command_queue d_q, cl_event
> > *rd_ev) try {> 
> >  }
> >  
> >  CLOVER_API cl_int
> > 
> > +clEnqueueMarkerWithWaitList(cl_command_queue d_q, cl_uint num_evs,
> > +                            const cl_event *d_evs, cl_event *rd_ev) try {
> > +   auto &q = obj(d_q);
> > +	 auto deps = objs<wait_list_tag>(d_evs, num_evs);
> > +
> > +	 for (auto &ev : deps) {
> 
> The indentation is weird here.

Sorry, I mostly use tabs and didn't see those remaining ones
> 
> > +      if (ev.context() != q.context())
> > +         throw error(CL_INVALID_CONTEXT);
> > +   }
> > +
> > +   // Create a hard event that depends on the events in the wait list.
> 
> I think it would be useful to mention here that this event is implicitly
> serialized with respect to the previous events in the command queue.

Ok, I'll keep the comment from clEnqueueWaitForEvents.
> 
> > +   auto hev = create<hard_event>(q, CL_COMMAND_MARKER, deps);
> > +
> > +   ret_object(rd_ev, hev);
> > +   return CL_SUCCESS;
> > +
> > +} catch (error &e) {
> > +   return e.get();
> > +}
> > +
> > +CLOVER_API cl_int
> > 
> >  clEnqueueBarrier(cl_command_queue d_q) try {
> >  
> >     obj(d_q);
> > 
> > @@ -191,21 +212,20 @@ clEnqueueBarrier(cl_command_queue d_q) try {
> > 
> >  }
> >  
> >  CLOVER_API cl_int
> > 
> > -clEnqueueWaitForEvents(cl_command_queue d_q, cl_uint num_evs,
> > -                       const cl_event *d_evs) try {
> > +clEnqueueBarrierWithWaitList(cl_command_queue d_q, cl_uint num_evs,
> > +                             const cl_event *d_evs, cl_event *rd_ev) try
> > {
> > 
> >     auto &q = obj(d_q);
> > 
> > -   auto evs = objs(d_evs, num_evs);
> > +	 auto deps = objs<wait_list_tag>(d_evs, num_evs);
> 
> The event list was named "evs" after its corresponding API objects
> d_evs, and num_evs.  It would be nice if you kept that correspondence,
> e.g. by renaming the entry-point arguments to d_deps and num_deps.  The
> same applies to clEnqueueMarkerWithWaitList().
> 
> > -   for (auto &ev : evs) {
> > +	 for (auto &ev : deps) {
> 
> Odd indentation again.

still my tabs setting
> 
> >        if (ev.context() != q.context())
> >        
> >           throw error(CL_INVALID_CONTEXT);
> >     
> >     }
> > 
> > -   // Create a hard event that depends on the events in the wait list:
> > -   // subsequent commands in the same queue will be implicitly
> > -   // serialized with respect to it -- hard events always are.
> > -   create<hard_event>(q, 0, evs);
> > +   // Create a hard event that depends on the events in the wait list.
> 
> I think the comment was more informative the way it was.

I will keep it
> 
> > +   auto hev = create<hard_event>(q, CL_COMMAND_BARRIER, deps);
> > 
> > +   ret_object(rd_ev, hev);
> > 
> >     return CL_SUCCESS;
> >  
> >  } catch (error &e) {
> > 
> > @@ -213,6 +233,18 @@ clEnqueueWaitForEvents(cl_command_queue d_q, cl_uint
> > num_evs,> 
> >  }
> >  
> >  CLOVER_API cl_int
> > 
> > +clEnqueueWaitForEvents(cl_command_queue d_q, cl_uint num_evs,
> > +                       const cl_event *d_evs) try {
> > +   // The wait list is mandatory for clEnqueueWaitForEvents().
> > +   objs(d_evs, num_evs);
> > +
> > +   return clEnqueueBarrierWithWaitList(d_q, num_evs, d_evs, NULL);
> > +
> > +} catch (error &e) {
> > +   return e.get();
> > +}
> > +
> > +CLOVER_API cl_int
> > 
> >  clGetEventProfilingInfo(cl_event d_ev, cl_profiling_info param,
> >  
> >                          size_t size, void *r_buf, size_t *r_size) try {
> >     
> >     property_buffer buf { r_buf, size, r_size };
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev