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?
> 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" <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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
