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

[Marek Olšák]

On Tue, Jan 10, 2012 at 6:20 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
>
>
> ----- Original Message -----
>> On Tue, Jan 10, 2012 at 5:15 PM, Jose Fonseca <jfonseca at vmware.com>
>> wrote:
>> > ----- Original Message -----
>> >> The flag is optional, it doesn't have to implemented by everybody.
>> >> If
>> >> we do the uploads in the state tracker, we will also do any
>> >> required
>> >> data transformation so that drivers don't have to do it at all.
>> >
>> > The same can be said for everything.
>> >
>> > I don't object adding yet another code path specific to a subset of
>> > hardware, provided the benefits justify its existence.
>> >
>> > But honestly I'm not yet convinced of this, as there was no attempt
>> > to backup with solid arguments why this matters.
>> >
>> > Furthermore this violates one of the principles gallium (and all 3D
>> > apis) have of unmapping all resources when drawing.
>>
>> If my idea violates one principle of all 3D APIs, this violates them
>> all:
>> http://www.opengl.org/registry/specs/AMD/pinned_memory.txt
>
> Indeed.
>
>> It allows reading user memory by hardware, and neither the driver nor
>> the hardware is notified when the memory contents are changed by the
>> user.
>>
>> BTW, I don't insist on this. I thought it would be a nice addition
>> allowing user buffer uploads to be moved out of (especially radeon)
>> drivers. If you really believe it's a bad optimization, feel free to
>> revert. WIthout it, things wouldn't change for me at all...
>
> Fair enough.
>
>> >
>> >> > It looks to me that state trackers and/or drivers are not
>> >> > properly
>> >> > using PIPE_USAGE_STREAM flag.  As all cases where
>> >> > PIPE_TRANSFER_MAP_PERMANENTLY would be used, the right way to do
>> >> > it would be for the state tracker to set PIPE_USAGE_STREAM, the
>> >> > pipe driver to recognize PIPE_USAGE_STREAM, and keep the mapping
>> >> > permanently internally, making mapping/unmapping of such buffers
>> >> > mere no-ops.
>> >>
>> >> We were doing that already and it wasn't good enough. Avoiding
>> >> create+map+unmap+destroy *calls* have brought higher frame rates
>> >> in
>> >> apps with lots of draw operations.
>> >
>> > I understand the number map/unmap call is reduced. But how does
>> > this interface change affect in any way the number of
>> > create/destroy calls?
>>
>> create = get_transfer
>> destroy = transfer_destroy
>
> Ah. BTW, we should probably unify create+map and destroy+unmap to reduce overhead. I would probably helps here.
>
>> > 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.
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.

Comments welcome.

Marek