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

[José Fonseca]

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

I see, good point. Separate u_upload_unmap u_upload_flush as you have
make all sense then.


Jose