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

Submitted by Francisco Jerez on March 19, 2018, 6:26 p.m.

Details

Message ID 20180319182659.22905-3-currojerez@riseup.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez March 19, 2018, 6:26 p.m.
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@lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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);
 }
 

Comments

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@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
>
Hey Nanley,

Nanley Chery <nanleychery@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@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
>>
On Fri, May 11, 2018 at 05:09:57PM -0700, Francisco Jerez wrote:
> Hey Nanley,
> 
> Nanley Chery <nanleychery@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@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@intel.com>

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