[Beignet] [PATCH v2] GetEventProfilingInfo could query all event status.

[Zhigang Gong]

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-bounces at lists.freedesktop.org] On Behalf Of
> Yang, Rong R
> Sent: Tuesday, August 4, 2015 3:31 PM
> To: Zhigang Gong; Luo, Xionghu
> Cc: beignet at 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-bounces at lists.freedesktop.org] On Behalf
> > Of Zhigang Gong
> > Sent: Tuesday, August 4, 2015 13:28
> > To: Luo, Xionghu
> > Cc: beignet at 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.gong at 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.gong at 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.luo at intel.com wrote:
> > > From: Luo Xionghu <xionghu.luo at 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.luo at 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 at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/beignet
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet