[Mesa-dev] [PATCH 2/5] radeonsi: fix error paths of si_texture_transfer_map
Nicolai Hähnle
nhaehnle at gmail.com
Fri Apr 20 07:19:59 UTC 2018
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.
More information about the mesa-dev
mailing list