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 

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(, texture, box, 
level, 0);
  if (!si_init_flushed_depth_texture(ctx, , 
_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, );

  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(, NULL);
  }
  /* Just get the strides. */
  si_texture_get_offset(sctx->screen, staging_depth, 
level, NULL,

  >b.b.stride,
  >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, 
_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,
   >b.b.stride,
@@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct 
pipe_context *ctx,

  si_init_temp_resource_from_box(, 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, );

  if (!staging) {
  PRINT_ERR("failed to create temporary texture to hold 
untiled copy\n");

-    FREE(trans);
-    return NULL;
+    goto fail_trans;
  }
  trans->staging = >resource;
  /* Just get the strides. */
  si_texture_get_offset(sctx->screen, staging, 0, NULL,
  >b.b.stride,
  >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,
   >b.b.stride,
   >b.b.layer_stride);
  buf = >resource;
  }
-    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage))) {
-    r600_resource_reference(>staging, NULL);
-    FREE(trans);
-    return NULL;
-    }
+    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage)))
+    goto fail_trans;
  *ptransfer = >b.b;
  return map + offset;
+
+fail_trans:
+    r600_resource_reference(>staging, NULL);


this needs to be:

if (trans->staging)
    r600_resource_reference(>staging, NULL);


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

Re: [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 

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(, texture, box, 
level, 0);
  if (!si_init_flushed_depth_texture(ctx, , 
_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, );

  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(, NULL);
  }
  /* Just get the strides. */
  si_texture_get_offset(sctx->screen, staging_depth, 
level, NULL,

  >b.b.stride,
  >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, 
_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,
   >b.b.stride,
@@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct 
pipe_context *ctx,

  si_init_temp_resource_from_box(, 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, );

  if (!staging) {
  PRINT_ERR("failed to create temporary texture to hold 
untiled copy\n");

-    FREE(trans);
-    return NULL;
+    goto fail_trans;
  }
  trans->staging = >resource;
  /* Just get the strides. */
  si_texture_get_offset(sctx->screen, staging, 0, NULL,
  >b.b.stride,
  >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,
   >b.b.stride,
   >b.b.layer_stride);
  buf = >resource;
  }
-    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage))) {
-    r600_resource_reference(>staging, NULL);
-    FREE(trans);
-    return NULL;
-    }
+    if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage)))
+    goto fail_trans;
  *ptransfer = >b.b;
  return map + offset;
+
+fail_trans:
+    r600_resource_reference(>staging, NULL);


this needs to be:

if (trans->staging)
    r600_resource_reference(>staging, NULL);


No, that's really not necessary. Only >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 

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

[Timothy Arceri]

On 11/04/18 20:56, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

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(, texture, box, level, 0);
  
  			if (!si_init_flushed_depth_texture(ctx, , _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, );
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(, NULL);
}
  
  			/* Just get the strides. */

si_texture_get_offset(sctx->screen, staging_depth, 
level, NULL,
>b.b.stride,
>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, 
_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,
 >b.b.stride,
@@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct 
pipe_context *ctx,
  
  		si_init_temp_resource_from_box(, 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, );
if (!staging) {
PRINT_ERR("failed to create temporary texture to hold 
untiled copy\n");
-   FREE(trans);
-   return NULL;
+   goto fail_trans;
}
trans->staging = >resource;
  
  		/* Just get the strides. */

si_texture_get_offset(sctx->screen, staging, 0, NULL,
>b.b.stride,
>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,
 >b.b.stride,
 >b.b.layer_stride);
buf = >resource;

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

[Nicolai Hähnle]

From: Nicolai Hähnle 

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(, texture, box, 
level, 0);
 
if (!si_init_flushed_depth_texture(ctx, , 
_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, );
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(, NULL);
}
 
/* Just get the strides. */
si_texture_get_offset(sctx->screen, staging_depth, 
level, NULL,
>b.b.stride,
>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, 
_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,
 >b.b.stride,
@@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct 
pipe_context *ctx,
 
si_init_temp_resource_from_box(, 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, );
if (!staging) {
PRINT_ERR("failed to create temporary texture to hold 
untiled copy\n");
-   FREE(trans);
-   return NULL;
+   goto fail_trans;
}
trans->staging = >resource;
 
/* Just get the strides. */
si_texture_get_offset(sctx->screen, staging, 0, NULL,
>b.b.stride,
>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,