[Mesa-dev] [PATCH 2/2] gallium: add flag PIPE_TRANSFER_MAP_PERMANENTLY

[Jose Fonseca]

----- Original Message -----
> On Don, 2012-01-26 at 20:45 +0100, Marek Olšák wrote:
> > On Tue, Jan 10, 2012 at 6:20 PM, Jose Fonseca <jfonseca at vmware.com>
> > wrote:
> > >> On Tue, Jan 10, 2012 at 5:15 PM, Jose Fonseca
> > >> <jfonseca at vmware.com>
> > >> wrote:
> > >> > 
> > >> > Also, please provide app name and performance figures w/ this
> > >> > change.
> > >>
> > >> OK. Torcs, the Forza track at the start.
> > >>
> > >> With u_upload_unmap before drawing:
> > >> 21.4 - 22.1 fps
> > >>
> > >> Without u_upload_unmap:
> > >> 22.7 - 23.1 fps
> > >
> > >
> > >> The improvement is approximately 1.1 fps, which is probably too
> > >> little
> > >> for other people to care, but why throw it away?
> > >
> > > This is roughly 5%. It is still significant.
> > >
> > > But it surprises me it is so high.
> > >
> > > Perhaps we should have a couple of fast entrypoints for
> > > transfering to/from buffers without the overhead of creating
> > > transfer objects.
> > 
> > Okay so the plan is to keep refactoring the transfer API until the
> > overhead disappears? Sounds good.
> > 
> > I propose these changes:
> > 1) Merge get_transfer with transfer_map.
> > 2) Merge transfer_unmap with transfer_destroy.
> 
> AFAIR everybody agreed a while ago this should be done.

I agree.

I know that in some places we pass transfer objects around, and then map/unmap elsewhere, which will make the refactoring harder, but it shouldn't be too hard.
 
> > 3) Make the pipe_transfer struct fully opaque, but then there must
> > be
> > another way to return the strides of mapped textures (out
> > parameters
> > of transfer_map?). Think of this as decoupling driver-private
> > transfer
> > objects from ordinary return values like the strides.
> > 4) The drivers which don't need transfer objects (e.g. non-texture
> > transfers) can return a NULL pipe_transfer struct, making transfer
> > objects fully optional. Only the returned pointer into the resource
> > determines whether transfer_map has been successful.
> > (1) and (2) should help reduce the call overhead. (3) is a
> > preparation
> > for (4). (4) should help kill all the code required to allocate,
> > initialize, and free the pipe_transfer struct when it's needed by
> > neither a driver nor a state tracker.
> 
> Sounds good to me.

Sounds great to me too.

One way to do this would be to make struct pipe_transfer a state tracker owned structure describing the transfer:

struct pipe_transfer
{
   struct pipe_resource *resource; /**< resource to transfer to/from  */
   unsigned level;                 /**< texture mipmap level */
   enum pipe_transfer_usage usage;
   struct pipe_box box;            /**< region of the resource to access */
   unsigned stride;                /**< row stride in bytes */
   unsigned layer_stride;          /**< image/layer stride in bytes */
   void *data;

   // Optional opaque driver private data associated with this transfer
   void *private;
};

And get/destroy_transfer would be disappear, being their functionality done inside map/unmap_transfer.  For example:

   struct pipe_transfer transfer;

   // TODO: write some inline helper functions to simplify most of this.
   transfer.resource = resource; // No need to reference count
   transfer.level = 0;
   transfer.usage = ...;
   ...

   if (pipe->map_transfer(pipe, &transfer)) {
       memcpy(transfer->data, buffer, size));
       pipe->unmap_transfer(pipe, &transfer);
   }

> Something else that might be useful at some point would be another
> entrypoint which passes in a pointer to the data to be transferred.
> This
> could allow saving a CPU copy in cases where the state tracker
> already
> has the data in the form appropriate for the transfer. For drivers
> that
> can't take advantage of that, there could be a utility function which
> uses the get_transfer and transfer_destroy hooks.

Yes. It would provide yet another way of doing transfers, but it could save intermediate copies on several hot spots.

Jose