<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> > 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. <div> </div> Jose <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Roland Scheidegger Sent: Monday, July 24, 2017 16:22 To: Jose Fonseca; mesa-dev@lists.freedesktop.org; Brian Paul; bruce.cherniak@intel.com Subject: Re: [PATCH] trace: Correct transfer box size calculation. <div> </div> </div> </div> <div class="PlainText">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@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); > </div> </div> </div> </body> </html>