Re: [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ć  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ć 
>>> ---
>>>  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() = 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() = 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jan Vesely]

On Fri, 2016-12-16 at 13:43 -0800, Francisco Jerez wrote:
> Vedran Miletić  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ć 
> > ---
> >  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() = 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() = 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

> >break;
> >  
> > default:
> > -- 
> > 2.7.4
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Jan Vesely 

signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Vedran Miletić  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ć 
> ---
>  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() = 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() = 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.

>break;
>  
> default:
> -- 
> 2.7.4


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

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ć 
---
 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() = 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() = ev.ref_count() - 1;
   break;
 
default:
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev