On Thu, 2017-09-14 at 14:39 -0700, Francisco Jerez wrote:
> Jan Vesely <jan.ves...@rutgers.edu> writes:
>
> > On Mon, 2017-09-04 at 13:23 -0700, Francisco Jerez wrote:
> > > Jan Vesely <jan.ves...@rutgers.edu> writes:
> > >
> > > > v2: wait in map_buffer and map_image as well
> > > > v3: use event::wait instead of wait (skips fence wait for hard_event)
> > > > v4: use wait_signalled()
> > > >
> > > > Signed-off-by: Jan Vesely <jan.ves...@rutgers.edu>
> > > > ---
> > > > Hi Francisco,
> > > >
> > > > once again sorry for the delay, and thanks for you patience.
> > > > This patch applies on top of the two you attached during our email
> > > > discussion.
> > > > From what I can tell, the functionality is identical to v3 after your
> > > > two patches are applied ("event:wait()" calls "wait_signalled()"), but I
> > > > suppose calling non-virtual function is preferrable. if not, feel free
> > > > to use v3.
> > > >
> > >
> > > Yeah, I find v4 more readable than calling the base class'
> > > implementation of wait(). Patch is:
> > >
> > > Reviewed-by: Francisco Jerez <curroje...@riseup.net>
> >
> > thanks. will you include with the other 2 patches, or should I push it
> > separately after those 2 are in?
> >
>
> I don't have review tags for the other two, but assuming they get your
> R-b feel free to push all the three patches yourself.
they look good to me, now that I understand how they work.
although I'd prefer external review given how long it took me.
@Aaron, do you mind taking a look at the two attached patches?
thanks,
Jan
>
> > regards,
> > Jan
> >
> > >
> > > Thanks.
> > >
> > > > thanks,
> > > > Jan
> > > >
> > > > src/gallium/state_trackers/clover/api/transfer.cpp | 30
> > > > ++++++++++++++++++++--
> > > > 1 file changed, 28 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/gallium/state_trackers/clover/api/transfer.cpp
> > > > b/src/gallium/state_trackers/clover/api/transfer.cpp
> > > > index f7046253be..34559042ae 100644
> > > > --- a/src/gallium/state_trackers/clover/api/transfer.cpp
> > > > +++ b/src/gallium/state_trackers/clover/api/transfer.cpp
> > > > @@ -295,6 +295,9 @@ clEnqueueReadBuffer(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > > &mem, obj_origin, obj_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -325,6 +328,9 @@ clEnqueueWriteBuffer(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > > ptr, {}, obj_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -362,6 +368,9 @@ clEnqueueReadBufferRect(cl_command_queue d_q,
> > > > cl_mem d_mem, cl_bool blocking,
> > > > &mem, obj_origin, obj_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -399,6 +408,9 @@ clEnqueueWriteBufferRect(cl_command_queue d_q,
> > > > cl_mem d_mem, cl_bool blocking,
> > > > ptr, host_origin, host_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -504,6 +516,9 @@ clEnqueueReadImage(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > > &img, src_origin, src_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -538,6 +553,9 @@ clEnqueueWriteImage(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > > ptr, {}, src_pitch,
> > > > region));
> > > >
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > ret_object(rd_ev, hev);
> > > > return CL_SUCCESS;
> > > >
> > > > @@ -667,7 +685,11 @@ clEnqueueMapBuffer(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > >
> > > > void *map = mem.resource(q).add_map(q, flags, blocking, obj_origin,
> > > > region);
> > > >
> > > > - ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_BUFFER,
> > > > deps));
> > > > + auto hev = create<hard_event>(q, CL_COMMAND_MAP_BUFFER, deps);
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > + ret_object(rd_ev, hev);
> > > > ret_error(r_errcode, CL_SUCCESS);
> > > > return map;
> > > >
> > > > @@ -695,7 +717,11 @@ clEnqueueMapImage(cl_command_queue d_q, cl_mem
> > > > d_mem, cl_bool blocking,
> > > >
> > > > void *map = img.resource(q).add_map(q, flags, blocking, origin,
> > > > region);
> > > >
> > > > - ret_object(rd_ev, create<hard_event>(q, CL_COMMAND_MAP_IMAGE,
> > > > deps));
> > > > + auto hev = create<hard_event>(q, CL_COMMAND_MAP_IMAGE, deps);
> > > > + if (blocking)
> > > > + hev().wait_signalled();
> > > > +
> > > > + ret_object(rd_ev, hev);
> > > > ret_error(r_errcode, CL_SUCCESS);
> > > > return map;
> > > >
> > > > --
> > > > 2.13.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
From bf951b0b3832355c336f5c340edf89fbfa797d94 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Tue, 9 Jun 2015 22:52:25 +0300
Subject: [Mesa-dev][PATCH 1/2] clover: Wrap event::wait_count in a method
taking care of the required locking.
---
src/gallium/state_trackers/clover/core/event.cpp | 19 ++++++++++++-------
src/gallium/state_trackers/clover/core/event.hpp | 3 ++-
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/gallium/state_trackers/clover/core/event.cpp b/src/gallium/state_trackers/clover/core/event.cpp
index 8275e16a4d..4f8531e407 100644
--- a/src/gallium/state_trackers/clover/core/event.cpp
+++ b/src/gallium/state_trackers/clover/core/event.cpp
@@ -27,7 +27,7 @@ using namespace clover;
event::event(clover::context &ctx, const ref_vector<event> &deps,
action action_ok, action action_fail) :
- context(ctx), wait_count(1), _status(0),
+ context(ctx), _wait_count(1), _status(0),
action_ok(action_ok), action_fail(action_fail) {
for (auto &ev : deps)
ev.chain(*this);
@@ -41,7 +41,7 @@ event::trigger_self() {
std::lock_guard<std::mutex> lock(mutex);
std::vector<intrusive_ref<event>> evs;
- if (!--wait_count)
+ if (!--_wait_count)
std::swap(_chain, evs);
return evs;
@@ -81,10 +81,15 @@ event::abort(cl_int status) {
ev.abort(status);
}
+unsigned
+event::wait_count() const {
+ std::lock_guard<std::mutex> lock(mutex);
+ return _wait_count;
+}
+
bool
event::signalled() const {
- std::lock_guard<std::mutex> lock(mutex);
- return !wait_count;
+ return !wait_count();
}
cl_int
@@ -99,8 +104,8 @@ event::chain(event &ev) {
std::unique_lock<std::mutex> lock_ev(ev.mutex, std::defer_lock);
std::lock(lock, lock_ev);
- if (wait_count) {
- ev.wait_count++;
+ if (_wait_count) {
+ ev._wait_count++;
_chain.push_back(ev);
}
ev.deps.push_back(*this);
@@ -112,7 +117,7 @@ event::wait() const {
ev.wait();
std::unique_lock<std::mutex> lock(mutex);
- cv.wait(lock, [=]{ return !wait_count; });
+ cv.wait(lock, [=]{ return !_wait_count; });
}
hard_event::hard_event(command_queue &q, cl_command_type command,
diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
index 6469e483c7..53dac686f8 100644
--- a/src/gallium/state_trackers/clover/core/event.hpp
+++ b/src/gallium/state_trackers/clover/core/event.hpp
@@ -85,8 +85,9 @@ namespace clover {
private:
std::vector<intrusive_ref<event>> trigger_self();
std::vector<intrusive_ref<event>> abort_self(cl_int status);
+ unsigned wait_count() const;
- unsigned wait_count;
+ unsigned _wait_count;
cl_int _status;
action action_ok;
action action_fail;
--
2.13.5
From 9bdcaaadc0390291aa7a2c34691bf5157a01ab3a Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Tue, 9 Jun 2015 22:59:43 +0300
Subject: [Mesa-dev][PATCH 2/2] clover: Run the associated action before an
event is signalled.
And define a method for other threads to wait until the action
function associated with an event has been executed to completion.
For hard events, this will mean waiting until the corresponding
command has been submitted to the pipe driver, without necessarily
flushing the pipe_context and waiting for the actual command to be
processed by the GPU (which is what hard_event::wait() already does).
This weaker kind of event wait will allow implementing blocking memory
transfers efficiently.
---
src/gallium/state_trackers/clover/core/event.cpp | 22 +++++++++++-----------
src/gallium/state_trackers/clover/core/event.hpp | 1 +
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/gallium/state_trackers/clover/core/event.cpp b/src/gallium/state_trackers/clover/core/event.cpp
index 4f8531e407..cd5d786604 100644
--- a/src/gallium/state_trackers/clover/core/event.cpp
+++ b/src/gallium/state_trackers/clover/core/event.cpp
@@ -44,19 +44,16 @@ event::trigger_self() {
if (!--_wait_count)
std::swap(_chain, evs);
+ cv.notify_all();
return evs;
}
void
event::trigger() {
- auto evs = trigger_self();
-
- if (signalled()) {
+ if (wait_count() == 1)
action_ok(*this);
- cv.notify_all();
- }
- for (event &ev : evs)
+ for (event &ev : trigger_self())
ev.trigger();
}
@@ -73,11 +70,9 @@ event::abort_self(cl_int status) {
void
event::abort(cl_int status) {
- auto evs = abort_self(status);
-
action_fail(*this);
- for (event &ev : evs)
+ for (event &ev : abort_self(status))
ev.abort(status);
}
@@ -112,12 +107,17 @@ event::chain(event &ev) {
}
void
+event::wait_signalled() const {
+ std::unique_lock<std::mutex> lock(mutex);
+ cv.wait(lock, [=]{ return !_wait_count; });
+}
+
+void
event::wait() const {
for (event &ev : deps)
ev.wait();
- std::unique_lock<std::mutex> lock(mutex);
- cv.wait(lock, [=]{ return !_wait_count; });
+ wait_signalled();
}
hard_event::hard_event(command_queue &q, cl_command_type command,
diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
index 53dac686f8..03c97bcf4d 100644
--- a/src/gallium/state_trackers/clover/core/event.hpp
+++ b/src/gallium/state_trackers/clover/core/event.hpp
@@ -69,6 +69,7 @@ namespace clover {
virtual cl_int status() const;
virtual command_queue *queue() const = 0;
virtual cl_command_type command() const = 0;
+ void wait_signalled() const;
virtual void wait() const;
virtual struct pipe_fence_handle *fence() const {
--
2.13.5
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
