On 03/10/2011 04:57 PM, José Fonseca wrote:
On Thu, 2011-03-10 at 06:01 -0800, Thomas Hellstrom wrote:
Make sure that the upload manager doesn't upload data that's not
dirty. This speeds up the viewperf test proe-04/1 a factor 5 or so on svga.
Sweet!
A few comments inline
Also introduce an u_upload_unmap() function that can be used
instead of u_upload_flush() so that we can pack
even more data in upload buffers. With this we can basically reuse the
upload buffer across flushes.
Signed-off-by: Thomas Hellstrom<thellst...@vmware.com>
---
src/gallium/auxiliary/util/u_upload_mgr.c | 41 ++++++++++++++++++++++------
src/gallium/auxiliary/util/u_upload_mgr.h | 10 +++++++
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_upload_mgr.c
b/src/gallium/auxiliary/util/u_upload_mgr.c
index dcf800a..7768e13 100644
--- a/src/gallium/auxiliary/util/u_upload_mgr.c
+++ b/src/gallium/auxiliary/util/u_upload_mgr.c
@@ -51,6 +51,7 @@ struct u_upload_mgr {
unsigned size; /* Actual size of the upload buffer. */
unsigned offset; /* Aligned offset to the upload buffer, pointing
* at the first unused byte. */
+ unsigned uploaded_offs; /* Offset below which data is already uploaded */
It's not clear the difference between offset and uploaded_offs. More
about this below.
};
@@ -72,6 +73,22 @@ struct u_upload_mgr *u_upload_create( struct pipe_context
*pipe,
return upload;
}
+void u_upload_unmap( struct u_upload_mgr *upload )
+{
+ if (upload->transfer) {
+ if (upload->size> upload->uploaded_offs) {
+ pipe_buffer_flush_mapped_range(upload->pipe, upload->transfer,
+ upload->uploaded_offs,
+ upload->offset -
upload->uploaded_offs);
+ }
+ pipe_transfer_unmap(upload->pipe, upload->transfer);
+ pipe_transfer_destroy(upload->pipe, upload->transfer);
+ upload->transfer = NULL;
+ upload->uploaded_offs = upload->offset;
+ upload->map = NULL;
+ }
+}
+
/* Release old buffer.
*
* This must usually be called prior to firing the command stream
@@ -84,17 +101,10 @@ struct u_upload_mgr *u_upload_create( struct pipe_context
*pipe,
void u_upload_flush( struct u_upload_mgr *upload )
{
/* Unmap and unreference the upload buffer. */
- if (upload->transfer) {
- if (upload->size) {
- pipe_buffer_flush_mapped_range(upload->pipe, upload->transfer,
- 0, upload->size);
- }
- pipe_transfer_unmap(upload->pipe, upload->transfer);
- pipe_transfer_destroy(upload->pipe, upload->transfer);
- upload->transfer = NULL;
- }
+ u_upload_unmap(upload);
pipe_resource_reference(&upload->buffer, NULL );
upload->size = 0;
+ upload->uploaded_offs = 0;
}
@@ -172,6 +182,19 @@ enum pipe_error u_upload_alloc( struct u_upload_mgr
*upload,
offset = MAX2(upload->offset, alloc_offset);
+ if (!upload->map) {
+ upload->map = pipe_buffer_map_range(upload->pipe, upload->buffer,
+ 0, upload->size,
+ PIPE_TRANSFER_WRITE |
+ PIPE_TRANSFER_FLUSH_EXPLICIT |
+ PIPE_TRANSFER_UNSYNCHRONIZED,
+ &upload->transfer);
+
Instead of the whole range, we should mapping only the area of
interest.
That is, of [0, upload->size], it should be [upload->uploaded_offs,
upload->size - uploaded_offs].
This mean that the driver / kernel does not need to populate the bytes
from [0, uploaded_offs] in the case where the contents is already gone
to VRAM.
I think uploaded_offs should be renamed to mapped_offset, i.e., "the
offset from which the buffer is currently mapped".
Agreed.
+ assert(offset>= upload->uploaded_offs);
+ upload->uploaded_offs = offset;
+ }
+
assert(offset< upload->buffer->width0);
assert(offset + size<= upload->buffer->width0);
assert(size);
diff --git a/src/gallium/auxiliary/util/u_upload_mgr.h
b/src/gallium/auxiliary/util/u_upload_mgr.h
index c9a2ffe..02426ea 100644
--- a/src/gallium/auxiliary/util/u_upload_mgr.h
+++ b/src/gallium/auxiliary/util/u_upload_mgr.h
@@ -67,6 +67,16 @@ void u_upload_destroy( struct u_upload_mgr *upload );
void u_upload_flush( struct u_upload_mgr *upload );
/**
+ * Unmap upload buffer
+ *
+ * \param upload Upload manager
+ *
+ * This is like u_upload_flush() except the upload buffer is kept for
+ * re-use across flushes. Allows us to pack more data into upload buffers.
+ */
+void u_upload_unmap( struct u_upload_mgr *upload );
These are two very different modes of operation.
When doing this, the uploaded buffer should be created with
PIPE_USAGE_STREAM and/or PIPE_USAGE_DYNAMIC (need to double check the
smantics of this), so that the driver can put this buffer in an
appropriate type of memory.
When not doing this, then the uploaded buffer should use
PIPE_USAGE_STATIC or IMMUTABLE (u_upload_mgr actually gets this wrong
now).
Instead of a new u_upload_unmap, I think that there should be a new flag
a u_upload_mgr create, or this should be done always. Is there really a
use case where we do not want to use this?
Yes. IMHO the difference is resource management in the general case.
Imagine a driver that is short on HW buffers for whatever reason. It
detects that and calls a context flush to eventually release more
buffers. That flush should release also the upload buffer, and the
driver should call u_upload_flush() instead of u_upload_unmap(). My
initial implementation in the SVGA driver used u_upload_unmap() for
hwtnl flushes and u_upload_flush for svga context flushes. This is more
clearly illustrated if the u_upload_manager were to be extended with an
optional small ring cache of hardware buffers.
/Thomas
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
