[PATCH] etnaviv: keep track of buffer valid ranges

[Christian Gmeiner]

Hi Lucas,

2017-10-19 11:25 GMT+02:00 Lucas Stach <l.stach at pengutronix.de>:
> Am Donnerstag, den 19.10.2017, 07:59 +0200 schrieb Christian Gmeiner:
>> This allows a write to proceed to an uninitialized part of a buffer
>> even when the GPU is using the previously-initialized portions.
>> Same is done for freedreno, nouveau and radeon.
>>
>> > Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
>> ---
>>  src/gallium/drivers/etnaviv/etnaviv_resource.c |  3 +++
>>  src/gallium/drivers/etnaviv/etnaviv_resource.h |  4 ++++
>>  src/gallium/drivers/etnaviv/etnaviv_transfer.c | 22 +++++++++++++++++++---
>>  3 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/drivers/etnaviv/etnaviv_resource.c b/src/gallium/drivers/etnaviv/etnaviv_resource.c
>> index d6cccd2dbb..4eb49bf936 100644
>> --- a/src/gallium/drivers/etnaviv/etnaviv_resource.c
>> +++ b/src/gallium/drivers/etnaviv/etnaviv_resource.c
>> @@ -268,6 +268,7 @@ etna_resource_alloc(struct pipe_screen *pscreen, unsigned layout,
>>
>>     pipe_reference_init(&rsc->base.reference, 1);
>>     list_inithead(&rsc->list);
>> +   util_range_init(&rsc->valid_buffer_range);
>>
>>     size = setup_miptree(rsc, paddingX, paddingY, msaa_xscale, msaa_yscale);
>>
>> @@ -458,6 +459,7 @@ etna_resource_destroy(struct pipe_screen *pscreen, struct pipe_resource *prsc)
>>     if (rsc->scanout)
>>        renderonly_scanout_destroy(rsc->scanout, etna_screen(pscreen)->ro);
>>
>> +   util_range_destroy(&rsc->valid_buffer_range);
>>     list_delinit(&rsc->list);
>>
>>     pipe_resource_reference(&rsc->texture, NULL);
>> @@ -494,6 +496,7 @@ etna_resource_from_handle(struct pipe_screen *pscreen,
>>
>>     pipe_reference_init(&prsc->reference, 1);
>>     list_inithead(&rsc->list);
>> +   util_range_init(&rsc->valid_buffer_range);
>>     prsc->screen = pscreen;
>>
>>     rsc->bo = etna_screen_bo_from_handle(pscreen, handle, &level->stride);
>> diff --git a/src/gallium/drivers/etnaviv/etnaviv_resource.h b/src/gallium/drivers/etnaviv/etnaviv_resource.h
>> index 0b135e2373..705c1d1af6 100644
>> --- a/src/gallium/drivers/etnaviv/etnaviv_resource.h
>> +++ b/src/gallium/drivers/etnaviv/etnaviv_resource.h
>> @@ -31,6 +31,7 @@
>>  #include "etnaviv_tiling.h"
>>  #include "pipe/p_state.h"
>>  #include "util/list.h"
>> +#include "util/u_range.h"
>>
>>  struct pipe_screen;
>>
>> @@ -73,6 +74,9 @@ struct etna_resource {
>>
>>     struct etna_resource_level levels[ETNA_NUM_LOD];
>>
>> +   /* buffer range that has been initialized */
>> +   struct util_range valid_buffer_range;
>> +
>>     /* When we are rendering to a texture, we need a differently tiled resource */
>>     struct pipe_resource *texture;
>>     /*
>> diff --git a/src/gallium/drivers/etnaviv/etnaviv_transfer.c b/src/gallium/drivers/etnaviv/etnaviv_transfer.c
>> index 6c1edd4835..33f3b058fa 100644
>> --- a/src/gallium/drivers/etnaviv/etnaviv_transfer.c
>> +++ b/src/gallium/drivers/etnaviv/etnaviv_transfer.c
>> @@ -126,6 +126,10 @@ etna_transfer_unmap(struct pipe_context *pctx, struct pipe_transfer *ptrans)
>>     if (!trans->rsc && !(ptrans->usage & PIPE_TRANSFER_UNSYNCHRONIZED))
>>        etna_bo_cpu_fini(rsc->bo);
>>
>> +   util_range_add(&rsc->valid_buffer_range,
>> +                  ptrans->box.x,
>> +                  ptrans->box.x + ptrans->box.width);
>> +
>>     pipe_resource_reference(&trans->rsc, NULL);
>>     pipe_resource_reference(&ptrans->resource, NULL);
>>     slab_free(&ctx->transfer_pool, trans);
>> @@ -283,7 +287,14 @@ etna_transfer_map(struct pipe_context *pctx, struct pipe_resource *prsc,
>>      * Pull resources into the CPU domain. Only skipped for unsynchronized
>>      * transfers without a temporary resource.
>>      */
>> -   if (trans->rsc || !(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
>> +   if ((usage & PIPE_TRANSFER_WRITE) &&
>> +         prsc->target == PIPE_BUFFER &&
>> +         !util_ranges_intersect(&rsc->valid_buffer_range,
>> +                                box->x, box->x + box->width)) {
>> +         /* We are trying to write to a previously uninitialized range. No need
>> +          * to wait.
>> +          */
>
> This unbalances the cpu_prep/fini in the map/unmap path. This isn't
> allowed and the kernel will start to reject this in the near future.
>

Good to know that the kernel will reject this behauvior in the near future.

> Also you need to differentiate between transfers with and without a
> temporary resource (trans->rsc), as the valid range of the temporary
> might be different from the base resource, especially if we need to
> read back from the base.
>

Thanks for the review - will rework it.

> Regards,
> Lucas
>
>> +    } else if (trans->rsc || !(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
>>        uint32_t prep_flags = 0;
>>
>>        if (usage & PIPE_TRANSFER_READ)
>> @@ -360,10 +371,15 @@ fail_prep:
>>
>>  static void
>>  etna_transfer_flush_region(struct pipe_context *pctx,
>> -                           struct pipe_transfer *transfer,
>> +                           struct pipe_transfer *ptrans,
>>                             const struct pipe_box *box)
>>  {
>> -   /* NOOP for now */
>> +   struct etna_resource *rsc = etna_resource(ptrans->resource);
>> +
>> +   if (ptrans->resource->target == PIPE_BUFFER)
>> +      util_range_add(&rsc->valid_buffer_range,
>> +                     ptrans->box.x + box->x,
>> +                     ptrans->box.x + box->x + box->width);
>>  }
>>
>>  void



greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info