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

[Cherniak, Bruce]

> On Jul 24, 2017, at 10:22 AM, Roland Scheidegger <sroland at vmware.com> wrote:
> 
> 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.

Very much appreciated.

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