Beignet current uses an on-demand manner to maintain the events' status. For those APIs which need to get current events' status, we insert a cl_gpgpu_event_update_status(). It seems there is no need for clFinish().
If you could find any case that if we don't insert it in clFinish() then it will cause functional error, I will agree that we need both patches. Even though xionghu's patch need to refine the commit log message, as it is not to fix GetEventProfilingInfo() issue. Thanks, Zhigang Gong. > -----Original Message----- > From: Beignet [mailto:beignet-boun...@lists.freedesktop.org] On Behalf Of > Yang, Rong R > Sent: Tuesday, August 4, 2015 3:31 PM > To: Zhigang Gong; Luo, Xionghu > Cc: beignet@lists.freedesktop.org > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event > status. > > I think these two patches both need when application GetEventProfilingInfo. > > From application view, it is make sense that all event that don't depend on > other events should have been flushed and finished when clFinish return. > The problem is the event have finished, but beignet don't update the event's > status, so I think it is better to update the event status before exit > clFinish. > > And GetEventProfilingInfo is another point need to update the event' status. > Suppose in the application, don't call clFinish, but maybe event has > completed, > should not return CL_PROFILING_INFO_NOT_AVAILABLE. > > > -----Original Message----- > > From: Beignet [mailto:beignet-boun...@lists.freedesktop.org] On Behalf > > Of Zhigang Gong > > Sent: Tuesday, August 4, 2015 13:28 > > To: Luo, Xionghu > > Cc: beignet@lists.freedesktop.org > > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query > > all event status. > > > > > > The spec only says if event is not in CL_COMPLETE status, it will get > > CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish() should > > update all events status. According to the spec, clFinish() only need > > to make sure the previously enqueued task has been processed and > completed. > > > > Form my point of view, if the application wants to call > > clGetEventProfilingInfo() to get an event's profiling information, the > > application should call > > clWaitForEvents() firstly. But given the fact that the specification > > is not very clear for whether clFinish() should update all events' > > status and some applications are really believe all events has been > > also updated after the > > clFinish() call. We may make a work around in Beignet to make those > > applications happy. Your patch is one solution and another may be > > better solution may be to add cl_event_update_status(event, 0) when > > calling to clGetEventProfilingInfo(). The patch is as below, > > > > From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00 > 2001 > > From: Zhigang Gong <zhigang.g...@intel.com> > > Date: Tue, 4 Aug 2015 13:21:27 +0800 > > Subject: [PATCH] runtime: always try to update event status in > > clGetEventProfilingInfo(). > > > > Some applications forgot to call clWaitForEvents() before calling to > > clGetEventProfilingInfo(). Let's update the event's status here. > > > > Signed-off-by: Zhigang Gong <zhigang.g...@intel.com> > > --- > > src/cl_api.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250 100644 > > --- a/src/cl_api.c > > +++ b/src/cl_api.c > > @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event > event, > > cl_ulong ret_val; > > > > CHECK_EVENT(event); > > + cl_event_update_status(event, 0); > > > > if (event->type == CL_COMMAND_USER || > > !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) || > > -- > > 1.9.1 > > > > > > This way we can avoid unecessary event updating in clFinish() call. > > The overhead is only introduced when the application calling > > clGetEventProfilingInfo() and we do not need busy wait for the event > > updating here. > > > > Thanks, > > Zhigang Gong. > > > > On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu....@intel.com wrote: > > > From: Luo Xionghu <xionghu....@intel.com> > > > > > > this could fix the shoc project reported > > > CL_PROFILING_INFO_NOT_AVAILABLE issue. > > > https://github.com/vetter/shoc/issues/47 > > > v2: per spec, the GetEventProfilingInfo need return > > > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not CL_COMPLETE, > > so > > > the event should be updated in clFinish. > > > > > > Signed-off-by: Luo Xionghu <xionghu....@intel.com> > > > --- > > > src/cl_command_queue.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index > > > 4e4ebfb..4b92311 100644 > > > --- a/src/cl_command_queue.c > > > +++ b/src/cl_command_queue.c > > > @@ -276,6 +276,9 @@ LOCAL cl_int > > > cl_command_queue_finish(cl_command_queue queue) { > > > cl_gpgpu_sync(cl_get_thread_batch_buf(queue)); > > > + cl_event last_event = get_last_event(queue); if (last_event) > > > + cl_event_update_status(last_event, 1); > > > return CL_SUCCESS; > > > } > > > > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Beignet mailing list > > > Beignet@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/beignet > > _______________________________________________ > > Beignet mailing list > > Beignet@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/beignet > _______________________________________________ > Beignet mailing list > Beignet@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list Beignet@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/beignet