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

[Timothy Arceri]

On 20/04/18 17:19, Nicolai Hähnle wrote:
> 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.

Yeah ignore the comment. I seem to have confused myself when I was 
reviewing this, on second look it seems fine.

> 
> 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;
>>>
> 
>