On Fri, May 11, 2018 at 05:09:57PM -0700, Francisco Jerez wrote:
> Hey Nanley,
>
> Nanley Chery <nanleych...@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-sta...@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.ch...@intel.com>
> >> }
> >>
> >> --
> >> 2.16.1
> >>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
