[Mesa-dev] [PATCH] gallium/util: Upload manager optimizations

[José Fonseca]

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 <thellstrom at 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".

> +      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?

Jose