[Mesa-dev] [PATCH 3/4] i965: Handle non-zero texture buffer offsets in buffer object range calculation.

[Nanley Chery]

On Fri, May 11, 2018 at 05:09:57PM -0700, Francisco Jerez wrote:
> Hey Nanley,
> 
> Nanley Chery <nanleychery at gmail.com> writes:
> 
> > On Mon, Mar 19, 2018 at 11:26:58AM -0700, Francisco Jerez wrote:
> >> Otherwise the specified surface state will allow the GPU to access
> >> memory up to BufferOffset bytes past the end of the buffer.  Found by
> >> inspection.
> >> 
> >> Cc: mesa-stable at lists.freedesktop.org
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> index ed4def9046e..2ab15af793a 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> @@ -654,7 +654,8 @@ buffer_texture_range_size(struct brw_context *brw,
> >>      * so that when ISL divides by stride to obtain the number of texels, that
> >>      * texel count is clamped to MAX_TEXTURE_BUFFER_SIZE.
> >>      */
> >> -   return MIN3((unsigned)obj->BufferSize, buffer_size,
> >> +   return MIN3((unsigned)obj->BufferSize,
> >> +               buffer_size - obj->BufferOffset,
> >>                 brw->ctx.Const.MaxTextureBufferSize * texel_size);
> >
> > I don't understand this change. Previously we took the minimum between:
> > 1) The TextureBuffer size specified by glTexBufferRange().
> > 2) The size of the buffer object specified by glTexBuffer().
> > 3) The maximum allowed texture buffer size.
> >
> > This patch modifies case 2 to be subtracted by the offset which will
> > always be 0 for glTexBuffer().
> >
> 
> The second argument of the MIN3 function is calculating the size of the
> buffer object range accessible to the GPU.  The correct offset interval
> that is supposed to be mapped to the GPU is [obj->BufferOffset,
> obj->BufferObject->Size[, from where the expression above follows.
> 

We discussed this in the office. The scenario in question is if the user
calls glTexBufferRange() with a non-zero offset, then shrinks the size
of the backing buffer object with glBufferData().

Thinking about some scenarios aloud here:
Scenario A:
* Create a texbuf s.t. texbuf_offset == 0 && texbuf_size > 0
* Shrink the backing buf s.t. buf_size < texbuf_size
* buffer_texture_range_size() returns buf_size pre and post patch (correct)
Scenario B:
* Create a texbuf s.t. texbuf_offset > 0 && texbuf_size > 0
* Shrink the backing buf s.t. buf_size < texbuf_offset && buf_size > texbuf_size
* buffer_texture_range_size() returns texbuf_size pre-patch (incorrect) and 
  buf_size - texbuf_offset post-patch (negative number -> incorrect). We
  should return 0.
Scenario C:
* Create a texbuf s.t. texbuf_offset > 0 && texbuf_size > 0
* Shrink the backing buf s.t. buf_size < texbuf_offset && buf_size < texbuf_size
* buffer_texture_range_size() returns buf_size pre-patch (incorrect) and 
  buf_size - texbuf_offset post-patch (negative number -> incorrect). We
  should return 0.
Scenario D:
* Create a texbuf s.t. texbuf_offset > 0 && texbuf_size > 0
* Shrink the backing buf s.t. buf_size > texbuf_offset && buf_size < texbuf_size
* buffer_texture_range_size() returns buf_size pre-patch (incorrect) and 
  buf_size - texbuf_offset post-patch (correct).
Scenario E:
* Create a texbuf s.t. texbuf_offset > 0 && texbuf_size > 0
* Shrink the backing buf s.t. buf_size > texbuf_offset && buf_size > texbuf_size
                              && buf_size < texbuf_offset + texbuf_size
* buffer_texture_range_size() returns texbuf_size pre-patch (incorrect) and 
  buf_size - texbuf_offset post-patch (correct).

This patch fixes scenarios D and E. I think it'd be helpful if you added an
assert or at least a comment about the cases this function doesn't
handle. With that, this patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>

> >>  }
> >>  
> >> -- 
> >> 2.16.1
> >>