[Mesa-dev] [PATCH 2/5] radeonsi: fix error paths of si_texture_transfer_map

[Nicolai Hähnle]

On 12.04.2018 02:10, Timothy Arceri wrote:
> 
> 
> On 11/04/18 20:56, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> trans is zero-initialized, but trans->resource is setup immediately so
>> needs to be dereferenced.
>> ---
>>   src/gallium/drivers/radeonsi/si_texture.c | 25 
>> ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_texture.c 
>> b/src/gallium/drivers/radeonsi/si_texture.c
>> index 0bab2a6c45b..907e8c8cec9 100644
>> --- a/src/gallium/drivers/radeonsi/si_texture.c
>> +++ b/src/gallium/drivers/radeonsi/si_texture.c
>> @@ -1728,49 +1728,46 @@ static void *si_texture_transfer_map(struct 
>> pipe_context *ctx,
>>                * then decompress the temporary one to staging.
>>                *
>>                * Only the region being mapped is transfered.
>>                */
>>               struct pipe_resource resource;
>>               si_init_temp_resource_from_box(&resource, texture, box, 
>> level, 0);
>>               if (!si_init_flushed_depth_texture(ctx, &resource, 
>> &staging_depth)) {
>>                   PRINT_ERR("failed to create temporary texture to 
>> hold untiled copy\n");
>> -                FREE(trans);
>> -                return NULL;
>> +                goto fail_trans;
>>               }
>>               if (usage & PIPE_TRANSFER_READ) {
>>                   struct pipe_resource *temp = 
>> ctx->screen->resource_create(ctx->screen, &resource);
>>                   if (!temp) {
>>                       PRINT_ERR("failed to create a temporary depth 
>> texture\n");
>> -                    FREE(trans);
>> -                    return NULL;
>> +                    goto fail_trans;
>>                   }
>>                   si_copy_region_with_blit(ctx, temp, 0, 0, 0, 0, 
>> texture, level, box);
>>                   si_blit_decompress_depth(ctx, (struct 
>> r600_texture*)temp, staging_depth,
>>                                0, 0, 0, box->depth, 0, 0);
>>                   pipe_resource_reference(&temp, NULL);
>>               }
>>               /* Just get the strides. */
>>               si_texture_get_offset(sctx->screen, staging_depth, 
>> level, NULL,
>>                           &trans->b.b.stride,
>>                           &trans->b.b.layer_stride);
>>           } else {
>>               /* XXX: only readback the rectangle which is being 
>> mapped? */
>>               /* XXX: when discard is true, no need to read back from 
>> depth texture */
>>               if (!si_init_flushed_depth_texture(ctx, texture, 
>> &staging_depth)) {
>>                   PRINT_ERR("failed to create temporary texture to 
>> hold untiled copy\n");
>> -                FREE(trans);
>> -                return NULL;
>> +                goto fail_trans;
>>               }
>>               si_blit_decompress_depth(ctx, rtex, staging_depth,
>>                            level, level,
>>                            box->z, box->z + box->depth - 1,
>>                            0, 0);
>>               offset = si_texture_get_offset(sctx->screen, staging_depth,
>>                                level, box,
>>                                &trans->b.b.stride,
>> @@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct 
>> pipe_context *ctx,
>>           si_init_temp_resource_from_box(&resource, texture, box, level,
>>                            SI_RESOURCE_FLAG_TRANSFER);
>>           resource.usage = (usage & PIPE_TRANSFER_READ) ?
>>               PIPE_USAGE_STAGING : PIPE_USAGE_STREAM;
>>           /* Create the temporary texture. */
>>           staging = (struct 
>> r600_texture*)ctx->screen->resource_create(ctx->screen, &resource);
>>           if (!staging) {
>>               PRINT_ERR("failed to create temporary texture to hold 
>> untiled copy\n");
>> -            FREE(trans);
>> -            return NULL;
>> +            goto fail_trans;
>>           }
>>           trans->staging = &staging->resource;
>>           /* Just get the strides. */
>>           si_texture_get_offset(sctx->screen, staging, 0, NULL,
>>                       &trans->b.b.stride,
>>                       &trans->b.b.layer_stride);
>>           if (usage & PIPE_TRANSFER_READ)
>>               si_copy_to_staging_texture(ctx, trans);
>> @@ -1809,28 +1805,31 @@ static void *si_texture_transfer_map(struct 
>> pipe_context *ctx,
>>           buf = trans->staging;
>>       } else {
>>           /* the resource is mapped directly */
>>           offset = si_texture_get_offset(sctx->screen, rtex, level, box,
>>                            &trans->b.b.stride,
>>                            &trans->b.b.layer_stride);
>>           buf = &rtex->resource;
>>       }
>> -    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage))) {
>> -        r600_resource_reference(&trans->staging, NULL);
>> -        FREE(trans);
>> -        return NULL;
>> -    }
>> +    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage)))
>> +        goto fail_trans;
>>       *ptransfer = &trans->b.b;
>>       return map + offset;
>> +
>> +fail_trans:
>> +    r600_resource_reference(&trans->staging, NULL);
> 
> this needs to be:
> 
> if (trans->staging)
>     r600_resource_reference(&trans->staging, NULL);

No, that's really not necessary. Only &trans->staging needs to be 
non-NULL. r600_resource_reference is basically an assignment with 
reference counting, both the old and the new value can be NULL 
simultaneously, see pipe_reference_described.

Cheers,
Nicolai

> 
> Otherwise:
> 
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
> 
>> +    pipe_resource_reference(&trans->b.b.resource, NULL);
>> +    FREE(trans);
>> +    return NULL;
>>   }
>>   static void si_texture_transfer_unmap(struct pipe_context *ctx,
>>                         struct pipe_transfer* transfer)
>>   {
>>       struct si_context *sctx = (struct si_context*)ctx;
>>       struct r600_transfer *rtransfer = (struct r600_transfer*)transfer;
>>       struct pipe_resource *texture = transfer->resource;
>>       struct r600_texture *rtex = (struct r600_texture*)texture;
>>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.