[Mesa-dev] [RFC PATCH] clover: Return correct CL_EVENT_REFERENCE_COUNT

[Vedran Miletić]

On 12/20/2016 11:59 AM, Jan Vesely wrote:
> On Fri, 2016-12-16 at 13:43 -0800, Francisco Jerez wrote:
>> Vedran Miletić <vedran at miletic.net> writes:
>>
>>> Current implementation of event handling keeps an extra reference to
>>> the hardware event, in addition to the reference returned via the OpenCL
>>> API. This additional reference is internal and should not be counted
>>> when queried via the clGetEventInfo() function.
>>>
>>> Fixes Piglit's cl/api/retain_release-event test.
>>>
>>> Signed-off-by: Vedran Miletić <vedran at miletic.net>
>>> ---
>>>  src/gallium/state_trackers/clover/api/event.cpp | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/state_trackers/clover/api/event.cpp b/src/gallium/state_trackers/clover/api/event.cpp
>>> index 5d1a0e5..74bc4d9 100644
>>> --- a/src/gallium/state_trackers/clover/api/event.cpp
>>> +++ b/src/gallium/state_trackers/clover/api/event.cpp
>>> @@ -107,7 +107,9 @@ clGetEventInfo(cl_event d_ev, cl_event_info param,
>>>        break;
>>>  
>>>     case CL_EVENT_REFERENCE_COUNT:
>>> -      buf.as_scalar<cl_uint>() = ev.ref_count();
>>> +      // Current implementation of event handling keeps an extra reference to
>>> +      // the hardware event, which is internal and should not be counted.
>>> +      buf.as_scalar<cl_uint>() = ev.ref_count() - 1;
>>
>> I don't think this is correct.  There is an internal event reference
>> held by the command queue object, but only for as long as the event
>> remains in the queue until the next flush.  In other cases the above
>> would give you a reference count which is off by one.  That said:
>>
>>> The reference count returned should be considered immediately
>>> stale. It is unsuitable for general use in applications. This feature
>>> is provided for identifying memory leaks.
> 
> 
> I found only a generic description that mentions reference count == 1
> wrt. events (in Glossary). Even there it says that it's an internal
> counter.
> The only object that seems to require reference count == 1 is root
> device. Contexts, queues, mem, samplers, programs, kernels, events, all
>  include the above footnote.
> I think the piglit test should be changed to check for non-zero value
> instead of 1.
> 
> Jan
> 

Thank you both for your inputs. Let's try changing the Piglit test.

Vedran

-- 
Vedran Miletić
vedran.miletic.net