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?

(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.

(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/

(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().

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>

(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?

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/1723.