Bug 106481 - No test for Image Load/Store on texture buffer sized greater than MAX_TEXTURE_BUFFER_SIZE_ARB
Summary: No test for Image Load/Store on texture buffer sized greater than MAX_TEXTURE...
Status: NEW
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Intel 3D Bugs Mailing List
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-11 22:53 UTC by Nanley Chery
Modified: 2018-09-07 17:48 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
piglit test workaround (775 bytes, text/plain)
2018-05-23 13:04 UTC, Illia Iorin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nanley Chery 2018-05-11 22:53:10 UTC
From the ARB_texture_buffer_object extension spec:

    While buffer textures can be substantially larger than equivalent
    one-dimensional textures; the maximum texture size supported for buffer
    textures in the initial implementation of this extension is 2^27 texels,
    versus 2^13 (8192) texels for otherwise equivalent one-dimensional
    textures.  [...]

There's a bug in i965, that allows the creation of buffer surface states with sizes greater than 2^27 texels when doing image load/store operations on texture buffers. This patch fixes the issue: https://patchwork.freedesktop.org/patch/211341/ 

There doesn't seem to be any piglit tests which exercise this path. We should create one to prevent this bug from showing up again.
Comment 1 Illia Iorin 2018-05-23 13:04:27 UTC
Created attachment 139707 [details]
piglit test workaround

Hello, i try to edit piglit test to cover that situation, and your mesa patch doesn't fix the issue.  My output is 

MAX_TEXTURE_BUFFER_SIZE: 134217728
Error code: 0
MAX_TEXTURE_BUFFER_SIZE: 134218728
Probe color at (0,0)
  Expected: 0 255 0 0
  Observed: 0 0 0 0
Probe color at (0,0)
  Expected: 0 255 0 0
  Observed: 0 0 0 0
Probe color at (0,0)
  Expected: 0 255 0 0
  Observed: 0 0 0 0
Probe color at (0,0)
  Expected: 0 255 0 0
  Observed: 0 0 0 0

Is need to make additional test witch cover that  behavior, or add assert to that test?
Comment 2 Nanley Chery 2018-05-30 21:19:14 UTC
(In reply to Illia from comment #1)
> Created attachment 139707 [details]
> piglit test workaround
> 
> Hello, i try to edit piglit test to cover that situation, and your mesa
> patch doesn't fix the issue.

Hi,

That's expected. The bug fix clamps the buffer size in the surface state. It doesn't modify glBufferData's behavior.

>  My output is 
> 
> MAX_TEXTURE_BUFFER_SIZE: 134217728
> Error code: 0
> MAX_TEXTURE_BUFFER_SIZE: 134218728
> Probe color at (0,0)
>   Expected: 0 255 0 0
>   Observed: 0 0 0 0
> Probe color at (0,0)
>   Expected: 0 255 0 0
>   Observed: 0 0 0 0
> Probe color at (0,0)
>   Expected: 0 255 0 0
>   Observed: 0 0 0 0
> Probe color at (0,0)
>   Expected: 0 255 0 0
>   Observed: 0 0 0 0
> 
> Is need to make additional test witch cover that  behavior, or add assert to
> that test?

I'm not sure what you'd assert in the max-size test. A new test can be created or an existing one modified. I'm guessing adding a test for GL_ARB_shader_image_size would be the easiest way to test the issue.
Comment 3 Illia Iorin 2018-06-12 17:07:07 UTC
(In reply to Nanley Chery from comment #2)
 
> I'm not sure what you'd assert in the max-size test. A new test can be
> created or an existing one modified. I'm guessing adding a test for
> GL_ARB_shader_image_size would be the easiest way to test the issue.

I have wrote  test to piglit https://patchwork.freedesktop.org/patch/227961/ that is the latest version. That test passes on radeon and fails on i965.  For some reason a new mail thread was created for that patch. Here is a link to original mail thread. https://patchwork.freedesktop.org/patch/227613/
Comment 4 Nanley Chery 2018-06-15 18:45:53 UTC
(In reply to Illia from comment #3)
> (In reply to Nanley Chery from comment #2)
>  
> > I'm not sure what you'd assert in the max-size test. A new test can be
> > created or an existing one modified. I'm guessing adding a test for
> > GL_ARB_shader_image_size would be the easiest way to test the issue.
> 
> I have wrote  test to piglit https://patchwork.freedesktop.org/patch/227961/
> that is the latest version. That test passes on radeon and fails on i965. 
> For some reason a new mail thread was created for that patch. Here is a link
> to original mail thread. https://patchwork.freedesktop.org/patch/227613/

Sorry, but this test doesn't cover the situation mentioned here. It's completely valid for a texture buffer to have a buffer object larger than 2^27B bound to it. The issue mentioned here was that the size field in the surface state object for buffers wasn't clamped to 2^27B as mentioned in the texture_buffer spec.  More information about these surface state objects can be found in the function, brw_emit_buffer_surface_state().
Comment 5 Andrés Gómez García 2018-09-07 17:25:20 UTC
Curro is this fixed by?

commit 156d2c6e621d836c4d45c636b87669e1de3d4464
Author: Francisco Jerez <currojerez@riseup.net>
Date:   Fri Mar 16 13:06:26 2018 -0700

    i965: Move buffer texture size calculation into a common helper function.
    
    The buffer texture size calculations (should be easy enough, right?)
    are repeated in three different places, each of them subtly broken in
    a different way.  E.g. the image load/store path was never fixed to
    clamp to MaxTextureBufferSize, and none of them are taking into
    account the buffer offset correctly.  It's easier to fix it all in one
    place.
    
    Cc: mesa-stable@lists.freedesktop.org
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106481
    Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>
Comment 6 Andrés Gómez García 2018-09-07 17:48:08 UTC
(In reply to Andrés Gómez García from comment #5)
> Curro is this fixed by?

[...]

My bad, I just realized that the report is about a missing piglit test.

In that case, shouldn't we move this report to the "piglit" product?


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.