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

Jan Vesely jan.vesely at rutgers.edu
Fri Mar 17 04:20:37 UTC 2017 

On Thu, 2017-03-16 at 18:07 -0700, Francisco Jerez wrote:
> Jan Vesely <jan.vesely at rutgers.edu> writes:
> 
> > On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote:
> > > Jan Vesely <jan.vesely at rutgers.edu> writes:
> > > 
> > > > On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote:
> > > > > Jan Vesely <jan.vesely at rutgers.edu> 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" <mesa-stable at lists.freedesktop.org>
> > > > > > 
> > > > > > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> > > > > > ---
> > > > > >  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 &dev, memory_obj &obj,
> > > > > >  }
> > > > > >  
> > > > > >  root_resource::~root_resource() {
> > > > > > -   device().pipe->resource_destroy(device().pipe, pipe);
> > > > > > +   pipe_resource_reference(&this->pipe, NULL);
> > > > > >  }
> > > > > >  
> > > > > >  sub_resource::sub_resource(resource &r, const vector &offset) :
> > > > > > @@ -202,18 +203,21 @@ mapping::mapping(command_queue &q, resource &r,
> > > > > >        pxfer = NULL;
> > > > > >        throw error(CL_OUT_OF_RESOURCES);
> > > > > >     }
> > > > > > +   pipe_resource_reference(&res, r.pipe);
> > > > > >  }
> > > > > >  
> > > > > >  mapping::mapping(mapping &&m) :
> > > > > > -   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(&res, 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

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170317/92fbec6a/attachment-0001.sig>

More information about the mesa-dev mailing list