> 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; [email protected]; Brian Paul; [email protected] 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 <[email protected]> > + 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); >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
