On 10.01.2012 12:29, Jose Fonseca wrote:
> Still catching up on email traffic during holidays...
>
> I agree that user buffer uploads should be moved out of drivers, but I don't
> think this is the way to go.
I don't. If the state tracker uploads user buffers and presents them to
the driver as normal buffers, it will have to change the
pipe_vertex_buffers for each draw, causing annoying re-validation
overhead. This is very bad if there are a lot of small draw calls.
Unfortunately many applications do this, and we do care about these cases.
Also, another example I'm dealing with atm, it will be difficult to
extract a small set of wide-spread vertices from a user buffer into a
oneshot vertex buffer (there are apps where this really helps a lot)
because you're changing the vertex index / gl_VertexID.
I can do that, because I know the hw has a special vertex-index buffer
(nvc0) or manual VERTEX_ID attribute (nv50) that can be routed to the
VERTEXID system value so I don't need to modify the shader.
> This "PIPE_TRANSFER_MAP_PERMANENTLY" means the driver relinquishes the
> ability to transform this data in any way before reashing the GPU -- i.e.,
> prevents any sort of workarounds -- something can't be always guaranteed.
> Flushing with map helps is also non-portable.
>
>
> 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.
>
>
> In summary, for me PIPE_TRANSFER_MAP_PERMANENTLY is premature/bad
> optmization. Before we are worried about saving a couple of map/unmap calls,
> we should ensure that PIPE_USAGE_STREAM/PIPE_USAGE_DYNAMIC code paths are
> fully optimal.
>
>
> Jose
>
> ----- Original Message -----
>> Please see the diff for further info.
>>
>> This paves the way for moving user buffer uploads out of drivers and
>> should
>> allow to clean up the mess in u_upload_mgr in the meantime.
>>
>> For now only allowed for buffers on r300 and r600.
>> ---
>> src/gallium/drivers/i915/i915_resource_buffer.c | 7 ++++++-
>> src/gallium/drivers/i915/i915_resource_texture.c | 7 ++++++-
>> src/gallium/drivers/llvmpipe/lp_texture.c | 4 ++++
>> src/gallium/drivers/nouveau/nouveau_buffer.c | 8 +++++++-
>> src/gallium/drivers/nv50/nv50_transfer.c | 2 +-
>> src/gallium/drivers/nvc0/nvc0_transfer.c | 2 +-
>> src/gallium/drivers/nvfx/nvfx_transfer.c | 3 +++
>> src/gallium/drivers/r300/r300_transfer.c | 4 ++++
>> src/gallium/drivers/r600/r600_texture.c | 4 ++++
>> src/gallium/drivers/svga/svga_resource_buffer.c | 4 ++++
>> src/gallium/drivers/svga/svga_resource_texture.c | 2 +-
>> src/gallium/include/pipe/p_defines.h | 16
>> ++++++++++++++++
>> 12 files changed, 57 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/gallium/drivers/i915/i915_resource_buffer.c
>> b/src/gallium/drivers/i915/i915_resource_buffer.c
>> index 77c0345..c54e481 100644
>> --- a/src/gallium/drivers/i915/i915_resource_buffer.c
>> +++ b/src/gallium/drivers/i915/i915_resource_buffer.c
>> @@ -68,8 +68,13 @@ i915_get_transfer(struct pipe_context *pipe,
>> const struct pipe_box *box)
>> {
>> struct i915_context *i915 = i915_context(pipe);
>> - struct pipe_transfer *transfer =
>> util_slab_alloc(&i915->transfer_pool);
>> + struct pipe_transfer *transfer;
>>
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> + transfer = util_slab_alloc(&i915->transfer_pool);
>> if (transfer == NULL)
>> return NULL;
>>
>> diff --git a/src/gallium/drivers/i915/i915_resource_texture.c
>> b/src/gallium/drivers/i915/i915_resource_texture.c
>> index 8ff733a..64d071c 100644
>> --- a/src/gallium/drivers/i915/i915_resource_texture.c
>> +++ b/src/gallium/drivers/i915/i915_resource_texture.c
>> @@ -720,9 +720,14 @@ i915_texture_get_transfer(struct pipe_context
>> *pipe,
>> {
>> struct i915_context *i915 = i915_context(pipe);
>> struct i915_texture *tex = i915_texture(resource);
>> - struct i915_transfer *transfer =
>> util_slab_alloc(&i915->texture_transfer_pool);
>> + struct i915_transfer *transfer;
>> boolean use_staging_texture = FALSE;
>>
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> + transfer = util_slab_alloc(&i915->texture_transfer_pool);
>> if (transfer == NULL)
>> return NULL;
>>
>> diff --git a/src/gallium/drivers/llvmpipe/lp_texture.c
>> b/src/gallium/drivers/llvmpipe/lp_texture.c
>> index ca38571..d86d493 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_texture.c
>> +++ b/src/gallium/drivers/llvmpipe/lp_texture.c
>> @@ -587,6 +587,10 @@ llvmpipe_get_transfer(struct pipe_context *pipe,
>> assert(resource);
>> assert(level <= resource->last_level);
>>
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> /*
>> * Transfers, like other pipe operations, must happen in order,
>> so flush the
>> * context if necessary.
>> diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.c
>> b/src/gallium/drivers/nouveau/nouveau_buffer.c
>> index f822625..02186ba 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_buffer.c
>> +++ b/src/gallium/drivers/nouveau/nouveau_buffer.c
>> @@ -172,7 +172,13 @@ nouveau_buffer_transfer_get(struct pipe_context
>> *pipe,
>> {
>> struct nv04_resource *buf = nv04_resource(resource);
>> struct nouveau_context *nv = nouveau_context(pipe);
>> - struct nouveau_transfer *xfr = CALLOC_STRUCT(nouveau_transfer);
>> + struct nouveau_transfer *xfr;
>> +
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> + xfr = CALLOC_STRUCT(nouveau_transfer);
>> if (!xfr)
>> return NULL;
>>
>> diff --git a/src/gallium/drivers/nv50/nv50_transfer.c
>> b/src/gallium/drivers/nv50/nv50_transfer.c
>> index 6f860e7..8ddebeb 100644
>> --- a/src/gallium/drivers/nv50/nv50_transfer.c
>> +++ b/src/gallium/drivers/nv50/nv50_transfer.c
>> @@ -243,7 +243,7 @@ nv50_miptree_transfer_new(struct pipe_context
>> *pctx,
>> uint32_t size;
>> int ret;
>>
>> - if (usage & PIPE_TRANSFER_MAP_DIRECTLY)
>> + if (usage & (PIPE_TRANSFER_MAP_DIRECTLY |
>> PIPE_TRANSFER_MAP_PERMANENTLY))
>> return NULL;
>>
>> tx = CALLOC_STRUCT(nv50_transfer);
>> diff --git a/src/gallium/drivers/nvc0/nvc0_transfer.c
>> b/src/gallium/drivers/nvc0/nvc0_transfer.c
>> index f168637..c04f41f 100644
>> --- a/src/gallium/drivers/nvc0/nvc0_transfer.c
>> +++ b/src/gallium/drivers/nvc0/nvc0_transfer.c
>> @@ -243,7 +243,7 @@ nvc0_miptree_transfer_new(struct pipe_context
>> *pctx,
>> uint32_t size;
>> int ret;
>>
>> - if (usage & PIPE_TRANSFER_MAP_DIRECTLY)
>> + if (usage & (PIPE_TRANSFER_MAP_DIRECTLY |
>> PIPE_TRANSFER_MAP_PERMANENTLY))
>> return NULL;
>>
>> tx = CALLOC_STRUCT(nvc0_transfer);
>> diff --git a/src/gallium/drivers/nvfx/nvfx_transfer.c
>> b/src/gallium/drivers/nvfx/nvfx_transfer.c
>> index 7a218b1..605af8e 100644
>> --- a/src/gallium/drivers/nvfx/nvfx_transfer.c
>> +++ b/src/gallium/drivers/nvfx/nvfx_transfer.c
>> @@ -26,6 +26,9 @@ nvfx_transfer_new(struct pipe_context *pipe,
>> unsigned usage,
>> const struct pipe_box *box)
>> {
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> if((usage & (PIPE_TRANSFER_UNSYNCHRONIZED |
>> PIPE_TRANSFER_DONTBLOCK)) == PIPE_TRANSFER_DONTBLOCK)
>> {
>> struct nouveau_bo* bo = ((struct
>> nvfx_resource*)pt)->bo;
>> diff --git a/src/gallium/drivers/r300/r300_transfer.c
>> b/src/gallium/drivers/r300/r300_transfer.c
>> index afdd530..91e861a 100644
>> --- a/src/gallium/drivers/r300/r300_transfer.c
>> +++ b/src/gallium/drivers/r300/r300_transfer.c
>> @@ -89,6 +89,10 @@ r300_texture_get_transfer(struct pipe_context
>> *ctx,
>> struct pipe_resource base;
>> boolean referenced_cs, referenced_hw;
>>
>> + if (usage & (PIPE_TRANSFER_MAP_DIRECTLY |
>> PIPE_TRANSFER_MAP_PERMANENTLY)) {
>> + return NULL;
>> + }
>> +
>> referenced_cs =
>> r300->rws->cs_is_buffer_referenced(r300->cs, tex->cs_buf);
>> if (referenced_cs) {
>> diff --git a/src/gallium/drivers/r600/r600_texture.c
>> b/src/gallium/drivers/r600/r600_texture.c
>> index 8fe54c8..decd941 100644
>> --- a/src/gallium/drivers/r600/r600_texture.c
>> +++ b/src/gallium/drivers/r600/r600_texture.c
>> @@ -630,6 +630,10 @@ struct pipe_transfer*
>> r600_texture_get_transfer(struct pipe_context *ctx,
>> int r;
>> boolean use_staging_texture = FALSE;
>>
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> /* We cannot map a tiled texture directly because the data is
>> * in a different order, therefore we do detiling using a blit.
>> *
>> diff --git a/src/gallium/drivers/svga/svga_resource_buffer.c
>> b/src/gallium/drivers/svga/svga_resource_buffer.c
>> index fa713ee..57ec752 100644
>> --- a/src/gallium/drivers/svga/svga_resource_buffer.c
>> +++ b/src/gallium/drivers/svga/svga_resource_buffer.c
>> @@ -74,6 +74,10 @@ svga_buffer_get_transfer(struct pipe_context
>> *pipe,
>> struct svga_buffer *sbuf = svga_buffer(resource);
>> struct pipe_transfer *transfer;
>>
>> + if (usage & PIPE_TRANSFER_MAP_PERMANENTLY) {
>> + return NULL;
>> + }
>> +
>> transfer = CALLOC_STRUCT(pipe_transfer);
>> if (transfer == NULL) {
>> return NULL;
>> diff --git a/src/gallium/drivers/svga/svga_resource_texture.c
>> b/src/gallium/drivers/svga/svga_resource_texture.c
>> index 697c1d3..4eb1068 100644
>> --- a/src/gallium/drivers/svga/svga_resource_texture.c
>> +++ b/src/gallium/drivers/svga/svga_resource_texture.c
>> @@ -259,7 +259,7 @@ svga_texture_get_transfer(struct pipe_context
>> *pipe,
>> unsigned nblocksy = util_format_get_nblocksy(texture->format,
>> box->height);
>>
>> /* We can't map texture storage directly */
>> - if (usage & PIPE_TRANSFER_MAP_DIRECTLY)
>> + if (usage & (PIPE_TRANSFER_MAP_DIRECTLY |
>> PIPE_TRANSFER_MAP_PERMANENTLY))
>> return NULL;
>>
>> assert(box->depth == 1);
>> diff --git a/src/gallium/include/pipe/p_defines.h
>> b/src/gallium/include/pipe/p_defines.h
>> index 3b3940d..91d8b1e 100644
>> --- a/src/gallium/include/pipe/p_defines.h
>> +++ b/src/gallium/include/pipe/p_defines.h
>> @@ -226,6 +226,22 @@ enum pipe_transfer_usage {
>> PIPE_TRANSFER_MAP_DIRECTLY = (1 << 2),
>>
>> /**
>> + * The transfer should map the resource storage directly and the
>> GPU should
>> + * be able to see what the CPU has written. Such a storage may
>> stay mapped
>> + * while issuing draw commands which use it. The only allowed
>> usage is
>> + * non-overlapping writes which are suballocated out of a big
>> buffer.
>> + * The minimum allowed alignment of suballocations is 256 bytes
>> (this is
>> + * a subject to change).
>> + * The flag is intended to be used to avoid mapping and unmapping
>> + * resources repeatedly when doing uploads and draws intermixed.
>> + *
>> + * The driver may return NULL if that isn't possible, and the
>> state
>> + * tracker needs to cope with that and use an alternative path
>> + * without this flag.
>> + */
>> + PIPE_TRANSFER_MAP_PERMANENTLY = (1 << 3),
>> +
>> + /**
>> * Discards the memory within the mapped region.
>> *
>> * It should not be used with PIPE_TRANSFER_READ.
>> --
>> 1.7.4.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
