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

[Thomas Hellstrom]

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

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