[Mesa-dev] [PATCH] trace: Correct transfer box size calculation.

[Jose Fonseca]

> This shouldn't be an issue, since buffers are required to have a 1-byte
format


Ah OK.  I was no longer sure.  In that case, I can leave the single code path and put an assert as you suggested.

Jose

________________________________
From: Roland Scheidegger
Sent: Monday, July 24, 2017 16:22
To: Jose Fonseca; mesa-dev at lists.freedesktop.org; Brian Paul; bruce.cherniak at intel.com
Subject: Re: [PATCH] trace: Correct transfer box size calculation.

Am 24.07.2017 um 15:26 schrieb Jose Fonseca:
> In particular:
>
> 1) For buffers the box expresses bytes, not pixels.
This shouldn't be an issue, since buffers are required to have a 1-byte
format (usually R8_UINT, but R8_UNORM works too).
(llvmpipe_transfer_map, for instance, will still do all the format dance
for calculating offsets, makes no difference either way.)


>
> 2) For textures we must not approximate the calculation with `stride *
>    height`, or `slice_stride * depth`, as that can easily lead to buffer
>    overflows, particularly for partial transfers.
>
>    But this code path is not currently very relevant since the trace
>    driver is only dumping data for buffers.
>
> This should address the issue that Bruce Cherniak found and diagnosed.


> ---
>  src/gallium/drivers/trace/tr_dump.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/src/gallium/drivers/trace/tr_dump.c b/src/gallium/drivers/trace/tr_dump.c
> index 78c72492dc..aab55644f3 100644
> --- a/src/gallium/drivers/trace/tr_dump.c
> +++ b/src/gallium/drivers/trace/tr_dump.c
> @@ -450,21 +450,25 @@ void trace_dump_box_bytes(const void *data,
>  {
>     size_t size;
>
> -   /*
> -    * Only dump buffer transfers to avoid huge files.
> -    * TODO: Make this run-time configurable
> -    */
> -   if (resource->target != PIPE_BUFFER) {
> -      size = 0;
> +   if (resource->target == PIPE_BUFFER) {
> +      /* For buffers the box should be in bytes, regardless of the format.
> +       */
So, maybe an assert that format is actually a 1-byte format would make
more sense instead.

But otherwise, looks great.
Reviewed-by: Roland Scheidegger <sroland at vmware.com>

> +      assert(box->height == 1);
> +      assert(box->depth == 1);
> +      size = box->width;
>     } else {
>        enum pipe_format format = resource->format;
> -      if (slice_stride)
> -         size = box->depth * slice_stride;
> -      else if (stride)
> -         size = util_format_get_nblocksy(format, box->height) * stride;
> -      else {
> -         size = util_format_get_nblocksx(format, box->width) * util_format_get_blocksize(format);
> -      }
> +      assert(box->height > 0);
> +      assert(box->depth > 0);
> +      size =  util_format_get_nblocksx(format, box->width )      * util_format_get_blocksize(format)
> +           + (util_format_get_nblocksy(format, box->height) - 1) * stride
> +           +                                  (box->depth   - 1) * slice_stride;
> +
> +      /*
> +       * Only dump buffer transfers to avoid huge files.
> +       * TODO: Make this run-time configurable
> +       */
> +      size = 0;
>     }
>
>     trace_dump_bytes(data, size);
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170724/4ca8c2fc/attachment.html>