Re: [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.haeh...@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 <tarc...@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;



_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev