On Fri, 2011-03-11 at 02:06 -0800, Thomas Hellstrom wrote:
> 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.
I see, good point. Separate u_upload_unmap u_upload_flush as you have
make all sense then.
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
