[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 <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.
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" <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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170316/df028d89/attachment.sig>