On Thu, 2017-03-16 at 18:07 -0700, Francisco Jerez wrote: > Jan Vesely <jan.ves...@rutgers.edu> writes: > > > On Thu, 2017-03-16 at 17:22 -0700, Francisco Jerez wrote: > > > Jan Vesely <jan.ves...@rutgers.edu> writes: > > > > > > > On Thu, 2017-03-16 at 15:24 -0700, Francisco Jerez wrote: > > > > > Jan Vesely <jan.ves...@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-sta...@lists.freedesktop.org> > > > > > > > > > > > > Signed-off-by: Jan Vesely <jan.ves...@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.ves...@rutgers.edu>
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
