Re: [Mesa-dev] [PATCH 3/4] i965: Handle non-zero texture buffer offsets in buffer object range calculation.
On Fri, May 11, 2018 at 05:09:57PM -0700, Francisco Jerez wrote: > Hey Nanley, > > Nanley Chery 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 > >> } > >> > >> -- > >> 2.16.1 > >> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/4] i965: Handle non-zero texture buffer offsets in buffer object range calculation.
Hey Nanley, Nanley Chery 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. >> } >> >> -- >> 2.16.1 >> signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/4] i965: Handle non-zero texture buffer offsets in buffer object range calculation.
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(). > } > > -- > 2.16.1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/4] i965: Handle non-zero texture buffer offsets in buffer object range calculation.
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); } -- 2.16.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
