Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Francisco Jerez]

Jan Vesely  writes:

> v2: buffers are created with one reference.
> v3: add pipe_resource reference to mapping object
>
> CC: "17.0 13.0" 
>
> Signed-off-by: Jan Vesely 
> ---
>  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
>  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
> b/src/gallium/state_trackers/clover/core/resource.cpp
> index 06fd3f6..83e3c26 100644
> --- a/src/gallium/state_trackers/clover/core/resource.cpp
> +++ b/src/gallium/state_trackers/clover/core/resource.cpp
> @@ -25,6 +25,7 @@
>  #include "pipe/p_screen.h"
>  #include "util/u_sampler.h"
>  #include "util/u_format.h"
> +#include "util/u_inlines.h"
>  
>  using namespace clover;
>  
> @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
> memory_obj ,
>  }
>  
>  root_resource::~root_resource() {
> -   device().pipe->resource_destroy(device().pipe, pipe);
> +   pipe_resource_reference(>pipe, NULL);
>  }
>  
>  sub_resource::sub_resource(resource , const vector ) :
> @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
>pxfer = NULL;
>throw error(CL_OUT_OF_RESOURCES);
> }
> +   pipe_resource_reference(, r.pipe);
>  }
>  
>  mapping::mapping(mapping &) :
> -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
> +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
> m.pctx = NULL;
> m.pxfer = NULL;
> +   m.res = NULL;
> m.p = NULL;
>  }
>  
>  mapping::~mapping() {
> if (pxfer) {
>pctx->transfer_unmap(pctx, pxfer);
> }
> +   pipe_resource_reference(, NULL);
>  }
>  
> @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
> std::swap(pctx, m.pctx);
> std::swap(pxfer, m.pxfer);
> +   std::swap(res, m.res);
> std::swap(p, m.p);
> return *this;
>  }
> diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
> b/src/gallium/state_trackers/clover/core/resource.hpp
> index 9993dcb..cea9617 100644
> --- a/src/gallium/state_trackers/clover/core/resource.hpp
> +++ b/src/gallium/state_trackers/clover/core/resource.hpp
> @@ -123,9 +123,10 @@ namespace clover {
>}
>  
> private:
> -  pipe_context *pctx;
> -  pipe_transfer *pxfer;
> -  void *p;
> +  pipe_context *pctx = NULL;
> +  pipe_transfer *pxfer = NULL;
> +  pipe_resource *res = NULL;

I'd rename this to 'pres' in order to match the naming of other data
members.  I'm generally okay with using inline member initializers but
the existing members are already getting initialized from the
constructor so this change seems kind of redundant and unrelated, you
could just initialize pres from the mapping::mapping constructor.

> +  void *p = NULL;
> };
>  }
>  
> -- 
> 2.9.3


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

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Francisco Jerez]

Vedran Miletić  writes:

> On 03/17/2017 05:20 AM, Jan Vesely wrote:
>> On Thu, 2017-03-16 at 18:07 -0700, Francisco Jerez wrote:
>>> Jan Vesely  writes:
>>>
 On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
> Jan Vesely  writes:
>
>> On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
>>> Jan Vesely  writes:
>>>
 v2: buffers are created with one reference.
 v3: add pipe_resource reference to mapping object

>>>
>>> Mapping objects are supposed to be short-lived, they're logically part
>>> of the parent resource object so they shouldn't ever out-live it.  What
>>> is this useful for?
>>
>> currently they can outlive the underlying pipe_resource. pipe_resource
>> is destroyed in root_resource destructor, while the list of mappings is
>> destroyed after resource destructor.
>
> Right.  I guess the problem is that the pipe_transfer object associated
> to the clover::mapping object holds a pointer to the backing
> pipe_resource object but it fails to increment its reference count?  I
> guess that's the reason why v2 didn't help?

 yes, though the pointer is hidden somewhere. I thought pxfer->resource
 might be it, but it's not, and digging deeper into the structure didn't
 sound like a good idea to me.

>>>
>>> What is pxfer->resource about in that case?
>> 
>> that's a good question, so I did a bit of digging. for EG global
>> buffers are shadowed in global buffer memory pool, so mapping uses
>> memory pool's pipe_resource. I'm still not 100% sure where exactly
>> unmapping the global pool accesses the original buffer's pipe_resource,
>> but I don't think it matters that much. It's not required for pxfer-
>>> resource to be equal to resource->pipe. we need to guarantee that
>> resource->pipe outlives all mappings.
>> either explicitly, or by grabbing reference.
>> 
>> Jan
>> 
>
> Jan's v3 solves the problem, as does my version.
>
> Francisco, do you have a particular preference how should we proceed?
>

I'm okay either way -- Though I'll send some codestyle-related
suggestions to Jan's patch in a minute.

> Regards,
> Vedran
>
> -- 
> Vedran Miletić
> vedran.miletic.net


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

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Vedran Miletić]

On 03/17/2017 05:20 AM, Jan Vesely wrote:
> On Thu, 2017-03-16 at 18:07 -0700, Francisco Jerez wrote:
>> Jan Vesely  writes:
>>
>>> On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
 Jan Vesely  writes:

> On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
>> Jan Vesely  writes:
>>
>>> v2: buffers are created with one reference.
>>> v3: add pipe_resource reference to mapping object
>>>
>>
>> Mapping objects are supposed to be short-lived, they're logically part
>> of the parent resource object so they shouldn't ever out-live it.  What
>> is this useful for?
>
> currently they can outlive the underlying pipe_resource. pipe_resource
> is destroyed in root_resource destructor, while the list of mappings is
> destroyed after resource destructor.

 Right.  I guess the problem is that the pipe_transfer object associated
 to the clover::mapping object holds a pointer to the backing
 pipe_resource object but it fails to increment its reference count?  I
 guess that's the reason why v2 didn't help?
>>>
>>> yes, though the pointer is hidden somewhere. I thought pxfer->resource
>>> might be it, but it's not, and digging deeper into the structure didn't
>>> sound like a good idea to me.
>>>
>>
>> What is pxfer->resource about in that case?
> 
> that's a good question, so I did a bit of digging. for EG global
> buffers are shadowed in global buffer memory pool, so mapping uses
> memory pool's pipe_resource. I'm still not 100% sure where exactly
> unmapping the global pool accesses the original buffer's pipe_resource,
> but I don't think it matters that much. It's not required for pxfer-
>> resource to be equal to resource->pipe. we need to guarantee that
> resource->pipe outlives all mappings.
> either explicitly, or by grabbing reference.
> 
> Jan
> 

Jan's v3 solves the problem, as does my version.

Francisco, do you have a particular preference how should we proceed?

Regards,
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] [PATCH v3 1/1] clover: use pipe_resource references

[Jan Vesely]

On Thu, 2017-03-16 at 18:07 -0700, Francisco Jerez wrote:
> Jan Vesely  writes:
> 
> > On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
> > > Jan Vesely  writes:
> > > 
> > > > On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
> > > > > Jan Vesely  writes:
> > > > > 
> > > > > > v2: buffers are created with one reference.
> > > > > > v3: add pipe_resource reference to mapping object
> > > > > > 
> > > > > 
> > > > > Mapping objects are supposed to be short-lived, they're logically part
> > > > > of the parent resource object so they shouldn't ever out-live it.  
> > > > > What
> > > > > is this useful for?
> > > > 
> > > > currently they can outlive the underlying pipe_resource. pipe_resource
> > > > is destroyed in root_resource destructor, while the list of mappings is
> > > > destroyed after resource destructor.
> > > 
> > > Right.  I guess the problem is that the pipe_transfer object associated
> > > to the clover::mapping object holds a pointer to the backing
> > > pipe_resource object but it fails to increment its reference count?  I
> > > guess that's the reason why v2 didn't help?
> > 
> > yes, though the pointer is hidden somewhere. I thought pxfer->resource
> > might be it, but it's not, and digging deeper into the structure didn't
> > sound like a good idea to me.
> > 
> 
> What is pxfer->resource about in that case?

that's a good question, so I did a bit of digging. for EG global
buffers are shadowed in global buffer memory pool, so mapping uses
memory pool's pipe_resource. I'm still not 100% sure where exactly
unmapping the global pool accesses the original buffer's pipe_resource,
but I don't think it matters that much. It's not required for pxfer-
>resource to be equal to resource->pipe. we need to guarantee that
resource->pipe outlives all mappings.
either explicitly, or by grabbing reference.

Jan


> 
> > > 
> > > > this is arguably an application bug. the piglit test does not call
> > > > clUnmapMemObject(), but it'd be nice to not access freed memory.
> > > > 
> > > > Vedran's alternative to clear the list before destroying pipe_resource
> > > > works as well (assert that the list is empty in resource destructor
> > > > would help spot the issue).
> > > > 
> > > 
> > > Assuming that pipe_transfers are supposed *not* to hold a reference to
> > > the underlying pipe_resource, which implies that the caller must
> > > guarantee it will never outlive its backing resource, it sounds like the
> > > minimal solution would be to have clover::mapping make the same
> > > assumptions.  You could probably achieve that in one line of code by
> > > clearing the mapping list from the clover::resource destructor as you
> > > suggest above.
> > 
> > I'd say the interface would be nicer if pipe_transfers did hold a
> > reference (or at least a mapping count to assert on), but I have no
> > plans to go that route.
> > the problem is a bit more complicated by the fact that pipe_resource is
> > handled by root_resource, while the list of mappings is private to
> > parent class resource.
> > 
> > Vedran's patch is here:
> > https://lists.freedesktop.org/archives/mesa-dev/2017-March/147092.html
> > 
> > I thought that using references would be nicer, as it looked useful for
> > device shared buffers, but that no longer applies.
> > 
> > Jan
> > 
> > > 
> > > > Jan
> > > > 
> > > > > 
> > > > > > CC: "17.0 13.0" 
> > > > > > 
> > > > > > Signed-off-by: Jan Vesely 
> > > > > > ---
> > > > > >  src/gallium/state_trackers/clover/core/resource.cpp | 11 
> > > > > > ---
> > > > > >  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
> > > > > >  2 files changed, 12 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
> > > > > > b/src/gallium/state_trackers/clover/core/resource.cpp
> > > > > > index 06fd3f6..83e3c26 100644
> > > > > > --- a/src/gallium/state_trackers/clover/core/resource.cpp
> > > > > > +++ b/src/gallium/state_trackers/clover/core/resource.cpp
> > > > > > @@ -25,6 +25,7 @@
> > > > > >  #include "pipe/p_screen.h"
> > > > > >  #include "util/u_sampler.h"
> > > > > >  #include "util/u_format.h"
> > > > > > +#include "util/u_inlines.h"
> > > > > >  
> > > > > >  using namespace clover;
> > > > > >  
> > > > > > @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device 
> > > > > > , memory_obj ,
> > > > > >  }
> > > > > >  
> > > > > >  root_resource::~root_resource() {
> > > > > > -   device().pipe->resource_destroy(device().pipe, pipe);
> > > > > > +   pipe_resource_reference(>pipe, NULL);
> > > > > >  }
> > > > > >  
> > > > > >  sub_resource::sub_resource(resource , const vector ) :
> > > > > > @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource 
> > > > > > ,
> > > > > >pxfer = NULL;
> > > > > >throw error(CL_OUT_OF_RESOURCES);
> 

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Francisco Jerez]

Jan Vesely  writes:

> On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
>> Jan Vesely  writes:
>> 
>> > On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
>> > > Jan Vesely  writes:
>> > > 
>> > > > v2: buffers are created with one reference.
>> > > > v3: add pipe_resource reference to mapping object
>> > > > 
>> > > 
>> > > Mapping objects are supposed to be short-lived, they're logically part
>> > > of the parent resource object so they shouldn't ever out-live it.  What
>> > > is this useful for?
>> > 
>> > currently they can outlive the underlying pipe_resource. pipe_resource
>> > is destroyed in root_resource destructor, while the list of mappings is
>> > destroyed after resource destructor.
>> 
>> Right.  I guess the problem is that the pipe_transfer object associated
>> to the clover::mapping object holds a pointer to the backing
>> pipe_resource object but it fails to increment its reference count?  I
>> guess that's the reason why v2 didn't help?
>
> yes, though the pointer is hidden somewhere. I thought pxfer->resource
> might be it, but it's not, and digging deeper into the structure didn't
> sound like a good idea to me.
>

What is pxfer->resource about in that case?

>> 
>> > this is arguably an application bug. the piglit test does not call
>> > clUnmapMemObject(), but it'd be nice to not access freed memory.
>> > 
>> > Vedran's alternative to clear the list before destroying pipe_resource
>> > works as well (assert that the list is empty in resource destructor
>> > would help spot the issue).
>> > 
>> 
>> Assuming that pipe_transfers are supposed *not* to hold a reference to
>> the underlying pipe_resource, which implies that the caller must
>> guarantee it will never outlive its backing resource, it sounds like the
>> minimal solution would be to have clover::mapping make the same
>> assumptions.  You could probably achieve that in one line of code by
>> clearing the mapping list from the clover::resource destructor as you
>> suggest above.
>
> I'd say the interface would be nicer if pipe_transfers did hold a
> reference (or at least a mapping count to assert on), but I have no
> plans to go that route.
> the problem is a bit more complicated by the fact that pipe_resource is
> handled by root_resource, while the list of mappings is private to
> parent class resource.
>
> Vedran's patch is here:
> https://lists.freedesktop.org/archives/mesa-dev/2017-March/147092.html
>
> I thought that using references would be nicer, as it looked useful for
> device shared buffers, but that no longer applies.
>
> Jan
>
>> 
>> > Jan
>> > 
>> > > 
>> > > > CC: "17.0 13.0" 
>> > > > 
>> > > > Signed-off-by: Jan Vesely 
>> > > > ---
>> > > >  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
>> > > >  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
>> > > >  2 files changed, 12 insertions(+), 6 deletions(-)
>> > > > 
>> > > > diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
>> > > > b/src/gallium/state_trackers/clover/core/resource.cpp
>> > > > index 06fd3f6..83e3c26 100644
>> > > > --- a/src/gallium/state_trackers/clover/core/resource.cpp
>> > > > +++ b/src/gallium/state_trackers/clover/core/resource.cpp
>> > > > @@ -25,6 +25,7 @@
>> > > >  #include "pipe/p_screen.h"
>> > > >  #include "util/u_sampler.h"
>> > > >  #include "util/u_format.h"
>> > > > +#include "util/u_inlines.h"
>> > > >  
>> > > >  using namespace clover;
>> > > >  
>> > > > @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
>> > > > memory_obj ,
>> > > >  }
>> > > >  
>> > > >  root_resource::~root_resource() {
>> > > > -   device().pipe->resource_destroy(device().pipe, pipe);
>> > > > +   pipe_resource_reference(>pipe, NULL);
>> > > >  }
>> > > >  
>> > > >  sub_resource::sub_resource(resource , const vector ) :
>> > > > @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
>> > > >pxfer = NULL;
>> > > >throw error(CL_OUT_OF_RESOURCES);
>> > > > }
>> > > > +   pipe_resource_reference(, r.pipe);
>> > > >  }
>> > > >  
>> > > >  mapping::mapping(mapping &) :
>> > > > -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
>> > > > +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
>> > > > m.pctx = NULL;
>> > > > m.pxfer = NULL;
>> > > > +   m.res = NULL;
>> > > > m.p = NULL;
>> > > >  }
>> > > >  
>> > > >  mapping::~mapping() {
>> > > > if (pxfer) {
>> > > >pctx->transfer_unmap(pctx, pxfer);
>> > > > }
>> > > > +   pipe_resource_reference(, NULL);
>> > > >  }
>> > > >  
>> > > > @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
>> > > > std::swap(pctx, m.pctx);
>> > > > std::swap(pxfer, m.pxfer);
>> > > > +   std::swap(res, m.res);
>> > > > std::swap(p, m.p);
>> > > > return *this;
>> > > >  }
>> > > > diff --git 

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Jan Vesely]

On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
> Jan Vesely  writes:
> 
> > On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
> > > Jan Vesely  writes:
> > > 
> > > > v2: buffers are created with one reference.
> > > > v3: add pipe_resource reference to mapping object
> > > > 
> > > 
> > > Mapping objects are supposed to be short-lived, they're logically part
> > > of the parent resource object so they shouldn't ever out-live it.  What
> > > is this useful for?
> > 
> > currently they can outlive the underlying pipe_resource. pipe_resource
> > is destroyed in root_resource destructor, while the list of mappings is
> > destroyed after resource destructor.
> 
> Right.  I guess the problem is that the pipe_transfer object associated
> to the clover::mapping object holds a pointer to the backing
> pipe_resource object but it fails to increment its reference count?  I
> guess that's the reason why v2 didn't help?

yes, though the pointer is hidden somewhere. I thought pxfer->resource
might be it, but it's not, and digging deeper into the structure didn't
sound like a good idea to me.

> 
> > this is arguably an application bug. the piglit test does not call
> > clUnmapMemObject(), but it'd be nice to not access freed memory.
> > 
> > Vedran's alternative to clear the list before destroying pipe_resource
> > works as well (assert that the list is empty in resource destructor
> > would help spot the issue).
> > 
> 
> Assuming that pipe_transfers are supposed *not* to hold a reference to
> the underlying pipe_resource, which implies that the caller must
> guarantee it will never outlive its backing resource, it sounds like the
> minimal solution would be to have clover::mapping make the same
> assumptions.  You could probably achieve that in one line of code by
> clearing the mapping list from the clover::resource destructor as you
> suggest above.

I'd say the interface would be nicer if pipe_transfers did hold a
reference (or at least a mapping count to assert on), but I have no
plans to go that route.
the problem is a bit more complicated by the fact that pipe_resource is
handled by root_resource, while the list of mappings is private to
parent class resource.

Vedran's patch is here:
https://lists.freedesktop.org/archives/mesa-dev/2017-March/147092.html

I thought that using references would be nicer, as it looked useful for
device shared buffers, but that no longer applies.

Jan

> 
> > Jan
> > 
> > > 
> > > > CC: "17.0 13.0" 
> > > > 
> > > > Signed-off-by: Jan Vesely 
> > > > ---
> > > >  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
> > > >  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
> > > >  2 files changed, 12 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
> > > > b/src/gallium/state_trackers/clover/core/resource.cpp
> > > > index 06fd3f6..83e3c26 100644
> > > > --- a/src/gallium/state_trackers/clover/core/resource.cpp
> > > > +++ b/src/gallium/state_trackers/clover/core/resource.cpp
> > > > @@ -25,6 +25,7 @@
> > > >  #include "pipe/p_screen.h"
> > > >  #include "util/u_sampler.h"
> > > >  #include "util/u_format.h"
> > > > +#include "util/u_inlines.h"
> > > >  
> > > >  using namespace clover;
> > > >  
> > > > @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
> > > > memory_obj ,
> > > >  }
> > > >  
> > > >  root_resource::~root_resource() {
> > > > -   device().pipe->resource_destroy(device().pipe, pipe);
> > > > +   pipe_resource_reference(>pipe, NULL);
> > > >  }
> > > >  
> > > >  sub_resource::sub_resource(resource , const vector ) :
> > > > @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
> > > >pxfer = NULL;
> > > >throw error(CL_OUT_OF_RESOURCES);
> > > > }
> > > > +   pipe_resource_reference(, r.pipe);
> > > >  }
> > > >  
> > > >  mapping::mapping(mapping &) :
> > > > -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
> > > > +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
> > > > m.pctx = NULL;
> > > > m.pxfer = NULL;
> > > > +   m.res = NULL;
> > > > m.p = NULL;
> > > >  }
> > > >  
> > > >  mapping::~mapping() {
> > > > if (pxfer) {
> > > >pctx->transfer_unmap(pctx, pxfer);
> > > > }
> > > > +   pipe_resource_reference(, NULL);
> > > >  }
> > > >  
> > > > @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
> > > > std::swap(pctx, m.pctx);
> > > > std::swap(pxfer, m.pxfer);
> > > > +   std::swap(res, m.res);
> > > > std::swap(p, m.p);
> > > > return *this;
> > > >  }
> > > > diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
> > > > b/src/gallium/state_trackers/clover/core/resource.hpp
> > > > index 9993dcb..cea9617 100644
> > > > --- a/src/gallium/state_trackers/clover/core/resource.hpp
> > > > +++ 

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Francisco Jerez]

Jan Vesely  writes:

> On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
>> Jan Vesely  writes:
>> 
>> > v2: buffers are created with one reference.
>> > v3: add pipe_resource reference to mapping object
>> > 
>> 
>> Mapping objects are supposed to be short-lived, they're logically part
>> of the parent resource object so they shouldn't ever out-live it.  What
>> is this useful for?
>
> currently they can outlive the underlying pipe_resource. pipe_resource
> is destroyed in root_resource destructor, while the list of mappings is
> destroyed after resource destructor.

Right.  I guess the problem is that the pipe_transfer object associated
to the clover::mapping object holds a pointer to the backing
pipe_resource object but it fails to increment its reference count?  I
guess that's the reason why v2 didn't help?

> this is arguably an application bug. the piglit test does not call
> clUnmapMemObject(), but it'd be nice to not access freed memory.
>
> Vedran's alternative to clear the list before destroying pipe_resource
> works as well (assert that the list is empty in resource destructor
> would help spot the issue).
>

Assuming that pipe_transfers are supposed *not* to hold a reference to
the underlying pipe_resource, which implies that the caller must
guarantee it will never outlive its backing resource, it sounds like the
minimal solution would be to have clover::mapping make the same
assumptions.  You could probably achieve that in one line of code by
clearing the mapping list from the clover::resource destructor as you
suggest above.

> Jan
>
>> 
>> > CC: "17.0 13.0" 
>> > 
>> > Signed-off-by: Jan Vesely 
>> > ---
>> >  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
>> >  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
>> >  2 files changed, 12 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
>> > b/src/gallium/state_trackers/clover/core/resource.cpp
>> > index 06fd3f6..83e3c26 100644
>> > --- a/src/gallium/state_trackers/clover/core/resource.cpp
>> > +++ b/src/gallium/state_trackers/clover/core/resource.cpp
>> > @@ -25,6 +25,7 @@
>> >  #include "pipe/p_screen.h"
>> >  #include "util/u_sampler.h"
>> >  #include "util/u_format.h"
>> > +#include "util/u_inlines.h"
>> >  
>> >  using namespace clover;
>> >  
>> > @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
>> > memory_obj ,
>> >  }
>> >  
>> >  root_resource::~root_resource() {
>> > -   device().pipe->resource_destroy(device().pipe, pipe);
>> > +   pipe_resource_reference(>pipe, NULL);
>> >  }
>> >  
>> >  sub_resource::sub_resource(resource , const vector ) :
>> > @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
>> >pxfer = NULL;
>> >throw error(CL_OUT_OF_RESOURCES);
>> > }
>> > +   pipe_resource_reference(, r.pipe);
>> >  }
>> >  
>> >  mapping::mapping(mapping &) :
>> > -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
>> > +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
>> > m.pctx = NULL;
>> > m.pxfer = NULL;
>> > +   m.res = NULL;
>> > m.p = NULL;
>> >  }
>> >  
>> >  mapping::~mapping() {
>> > if (pxfer) {
>> >pctx->transfer_unmap(pctx, pxfer);
>> > }
>> > +   pipe_resource_reference(, NULL);
>> >  }
>> >  
>> > @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
>> > std::swap(pctx, m.pctx);
>> > std::swap(pxfer, m.pxfer);
>> > +   std::swap(res, m.res);
>> > std::swap(p, m.p);
>> > return *this;
>> >  }
>> > diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
>> > b/src/gallium/state_trackers/clover/core/resource.hpp
>> > index 9993dcb..cea9617 100644
>> > --- a/src/gallium/state_trackers/clover/core/resource.hpp
>> > +++ b/src/gallium/state_trackers/clover/core/resource.hpp
>> > @@ -123,9 +123,10 @@ namespace clover {
>> >}
>> >  
>> > private:
>> > -  pipe_context *pctx;
>> > -  pipe_transfer *pxfer;
>> > -  void *p;
>> > +  pipe_context *pctx = NULL;
>> > +  pipe_transfer *pxfer = NULL;
>> > +  pipe_resource *res = NULL;
>> > +  void *p = NULL;
>> > };
>> >  }
>> >  
>> > -- 
>> > 2.9.3


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

Re: [Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Jan Vesely]

On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
> Jan Vesely  writes:
> 
> > v2: buffers are created with one reference.
> > v3: add pipe_resource reference to mapping object
> > 
> 
> Mapping objects are supposed to be short-lived, they're logically part
> of the parent resource object so they shouldn't ever out-live it.  What
> is this useful for?

currently they can outlive the underlying pipe_resource. pipe_resource
is destroyed in root_resource destructor, while the list of mappings is
destroyed after resource destructor.
this is arguably an application bug. the piglit test does not call
clUnmapMemObject(), but it'd be nice to not access freed memory.

Vedran's alternative to clear the list before destroying pipe_resource
works as well (assert that the list is empty in resource destructor
would help spot the issue).

Jan

> 
> > CC: "17.0 13.0" 
> > 
> > Signed-off-by: Jan Vesely 
> > ---
> >  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
> >  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
> > b/src/gallium/state_trackers/clover/core/resource.cpp
> > index 06fd3f6..83e3c26 100644
> > --- a/src/gallium/state_trackers/clover/core/resource.cpp
> > +++ b/src/gallium/state_trackers/clover/core/resource.cpp
> > @@ -25,6 +25,7 @@
> >  #include "pipe/p_screen.h"
> >  #include "util/u_sampler.h"
> >  #include "util/u_format.h"
> > +#include "util/u_inlines.h"
> >  
> >  using namespace clover;
> >  
> > @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
> > memory_obj ,
> >  }
> >  
> >  root_resource::~root_resource() {
> > -   device().pipe->resource_destroy(device().pipe, pipe);
> > +   pipe_resource_reference(>pipe, NULL);
> >  }
> >  
> >  sub_resource::sub_resource(resource , const vector ) :
> > @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
> >pxfer = NULL;
> >throw error(CL_OUT_OF_RESOURCES);
> > }
> > +   pipe_resource_reference(, r.pipe);
> >  }
> >  
> >  mapping::mapping(mapping &) :
> > -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
> > +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
> > m.pctx = NULL;
> > m.pxfer = NULL;
> > +   m.res = NULL;
> > m.p = NULL;
> >  }
> >  
> >  mapping::~mapping() {
> > if (pxfer) {
> >pctx->transfer_unmap(pctx, pxfer);
> > }
> > +   pipe_resource_reference(, NULL);
> >  }
> >  
> > @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
> > std::swap(pctx, m.pctx);
> > std::swap(pxfer, m.pxfer);
> > +   std::swap(res, m.res);
> > std::swap(p, m.p);
> > return *this;
> >  }
> > diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
> > b/src/gallium/state_trackers/clover/core/resource.hpp
> > index 9993dcb..cea9617 100644
> > --- a/src/gallium/state_trackers/clover/core/resource.hpp
> > +++ b/src/gallium/state_trackers/clover/core/resource.hpp
> > @@ -123,9 +123,10 @@ namespace clover {
> >}
> >  
> > private:
> > -  pipe_context *pctx;
> > -  pipe_transfer *pxfer;
> > -  void *p;
> > +  pipe_context *pctx = NULL;
> > +  pipe_transfer *pxfer = NULL;
> > +  pipe_resource *res = NULL;
> > +  void *p = NULL;
> > };
> >  }
> >  
> > -- 
> > 2.9.3


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] [PATCH v3 1/1] clover: use pipe_resource references

[Francisco Jerez]

Jan Vesely  writes:

> v2: buffers are created with one reference.
> v3: add pipe_resource reference to mapping object
>

Mapping objects are supposed to be short-lived, they're logically part
of the parent resource object so they shouldn't ever out-live it.  What
is this useful for?

> CC: "17.0 13.0" 
>
> Signed-off-by: Jan Vesely 
> ---
>  src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
>  src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
> b/src/gallium/state_trackers/clover/core/resource.cpp
> index 06fd3f6..83e3c26 100644
> --- a/src/gallium/state_trackers/clover/core/resource.cpp
> +++ b/src/gallium/state_trackers/clover/core/resource.cpp
> @@ -25,6 +25,7 @@
>  #include "pipe/p_screen.h"
>  #include "util/u_sampler.h"
>  #include "util/u_format.h"
> +#include "util/u_inlines.h"
>  
>  using namespace clover;
>  
> @@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
> memory_obj ,
>  }
>  
>  root_resource::~root_resource() {
> -   device().pipe->resource_destroy(device().pipe, pipe);
> +   pipe_resource_reference(>pipe, NULL);
>  }
>  
>  sub_resource::sub_resource(resource , const vector ) :
> @@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
>pxfer = NULL;
>throw error(CL_OUT_OF_RESOURCES);
> }
> +   pipe_resource_reference(, r.pipe);
>  }
>  
>  mapping::mapping(mapping &) :
> -   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
> +   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
> m.pctx = NULL;
> m.pxfer = NULL;
> +   m.res = NULL;
> m.p = NULL;
>  }
>  
>  mapping::~mapping() {
> if (pxfer) {
>pctx->transfer_unmap(pctx, pxfer);
> }
> +   pipe_resource_reference(, NULL);
>  }
>  
> @@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
> std::swap(pctx, m.pctx);
> std::swap(pxfer, m.pxfer);
> +   std::swap(res, m.res);
> std::swap(p, m.p);
> return *this;
>  }
> diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
> b/src/gallium/state_trackers/clover/core/resource.hpp
> index 9993dcb..cea9617 100644
> --- a/src/gallium/state_trackers/clover/core/resource.hpp
> +++ b/src/gallium/state_trackers/clover/core/resource.hpp
> @@ -123,9 +123,10 @@ namespace clover {
>}
>  
> private:
> -  pipe_context *pctx;
> -  pipe_transfer *pxfer;
> -  void *p;
> +  pipe_context *pctx = NULL;
> +  pipe_transfer *pxfer = NULL;
> +  pipe_resource *res = NULL;
> +  void *p = NULL;
> };
>  }
>  
> -- 
> 2.9.3


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

[Mesa-dev] [PATCH v3 1/1] clover: use pipe_resource references

[Jan Vesely]

v2: buffers are created with one reference.
v3: add pipe_resource reference to mapping object

CC: "17.0 13.0" 

Signed-off-by: Jan Vesely 
---
 src/gallium/state_trackers/clover/core/resource.cpp | 11 ---
 src/gallium/state_trackers/clover/core/resource.hpp |  7 ---
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/gallium/state_trackers/clover/core/resource.cpp 
b/src/gallium/state_trackers/clover/core/resource.cpp
index 06fd3f6..83e3c26 100644
--- a/src/gallium/state_trackers/clover/core/resource.cpp
+++ b/src/gallium/state_trackers/clover/core/resource.cpp
@@ -25,6 +25,7 @@
 #include "pipe/p_screen.h"
 #include "util/u_sampler.h"
 #include "util/u_format.h"
+#include "util/u_inlines.h"
 
 using namespace clover;
 
@@ -176,7 +177,7 @@ root_resource::root_resource(clover::device , 
memory_obj ,
 }
 
 root_resource::~root_resource() {
-   device().pipe->resource_destroy(device().pipe, pipe);
+   pipe_resource_reference(>pipe, NULL);
 }
 
 sub_resource::sub_resource(resource , const vector ) :
@@ -202,18 +203,21 @@ mapping::mapping(command_queue , resource ,
   pxfer = NULL;
   throw error(CL_OUT_OF_RESOURCES);
}
+   pipe_resource_reference(, r.pipe);
 }
 
 mapping::mapping(mapping &) :
-   pctx(m.pctx), pxfer(m.pxfer), p(m.p) {
+   pctx(m.pctx), pxfer(m.pxfer), res(m.res), p(m.p) {
m.pctx = NULL;
m.pxfer = NULL;
+   m.res = NULL;
m.p = NULL;
 }
 
 mapping::~mapping() {
if (pxfer) {
   pctx->transfer_unmap(pctx, pxfer);
}
+   pipe_resource_reference(, NULL);
 }
 
@@ -222,5 +226,6 @@ mapping::operator=(mapping m) {
std::swap(pctx, m.pctx);
std::swap(pxfer, m.pxfer);
+   std::swap(res, m.res);
std::swap(p, m.p);
return *this;
 }
diff --git a/src/gallium/state_trackers/clover/core/resource.hpp 
b/src/gallium/state_trackers/clover/core/resource.hpp
index 9993dcb..cea9617 100644
--- a/src/gallium/state_trackers/clover/core/resource.hpp
+++ b/src/gallium/state_trackers/clover/core/resource.hpp
@@ -123,9 +123,10 @@ namespace clover {
   }
 
private:
-  pipe_context *pctx;
-  pipe_transfer *pxfer;
-  void *p;
+  pipe_context *pctx = NULL;
+  pipe_transfer *pxfer = NULL;
+  pipe_resource *res = NULL;
+  void *p = NULL;
};
 }
 
-- 
2.9.3

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