On 2015-06-09 00:30:45, Samuel Iglesias Gonsálvez wrote:
> On 09/06/15 08:18, Samuel Iglesias Gonsálvez wrote:
> > On 08/06/15 23:36, Jordan Justen wrote:
> >> On 2015-06-03 00:00:59, Iago Toral Quiroga wrote:
> >>> This includes the array of bindings, the current buffer bound to the
> >>> GL_SHADER_STORAGE_BUFFER target and a set of general limits and default
> >>> values for shader storage buffers.
> >>> ---
> >>> src/mesa/main/bufferobj.c | 5 +++++
> >>> src/mesa/main/config.h | 2 ++
> >>> src/mesa/main/context.c | 6 ++++++
> >>> src/mesa/main/mtypes.h | 38 ++++++++++++++++++++++++++++++++++++++
> >>> 4 files changed, 51 insertions(+)
> >>>
> >>> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> >>> index 66dee68..c5d4ada 100644
> >>> --- a/src/mesa/main/bufferobj.c
> >>> +++ b/src/mesa/main/bufferobj.c
> >>> @@ -112,6 +112,11 @@ get_buffer_target(struct gl_context *ctx, GLenum
> >>> target)
> >>> return &ctx->UniformBuffer;
> >>> }
> >>> break;
> >>> + case GL_SHADER_STORAGE_BUFFER:
> >>> + if (ctx->Extensions.ARB_shader_storage_buffer_object) {
> >>> + return &ctx->ShaderStorageBuffer;
> >>> + }
> >>> + break;
> >>> case GL_ATOMIC_COUNTER_BUFFER:
> >>> if (ctx->Extensions.ARB_shader_atomic_counters) {
> >>> return &ctx->AtomicBuffer;
> >>> diff --git a/src/mesa/main/config.h b/src/mesa/main/config.h
> >>> index 5a66a4e..551aad1 100644
> >>> --- a/src/mesa/main/config.h
> >>> +++ b/src/mesa/main/config.h
> >>> @@ -171,8 +171,10 @@
> >>> #define MAX_PROGRAM_LOCAL_PARAMS 4096
> >>> #define MAX_UNIFORMS 4096
> >>> #define MAX_UNIFORM_BUFFERS 15 /* + 1 default uniform buffer
> >>> */
> >>> +#define MAX_SHADER_STORAGE_BUFFERS 15
> >>
> >> This is just to copy the ubo value?
> >>
> >
> > Yes, it is. We started copying the values from UBOs, like
> > consts->MaxShaderStorageBufferBindings (copied from
> > consts->MaxUniformBufferBindings) and similarly for others.
> >
> > We explained that in the cover letter of the first version of the patch
> > series [0]:
> >
> > "Both Mesa and i965 need to give default values for certain things
> > like the maximum allowed size of a shader storage buffer, the maximum
> > number of buffer bindings, the maximum number of combined shader storage
> > blocks, etc. We are not sure about what default values we should use
> > for these in all cases and except in a few cases we generally copied the
> > default values from uniforms. That may be fine or not, so let us know if
> > we should use different values for any of these setting at the Mesa or
> > i965 levels."
> >
> > [0] http://lists.freedesktop.org/archives/mesa-dev/2015-May/084196.html
> >
> >>> /* 6 is for vertex, hull, domain, geometry, fragment, and compute
> >>> shader. */
> >>> #define MAX_COMBINED_UNIFORM_BUFFERS (MAX_UNIFORM_BUFFERS * 6)
> >>> +#define MAX_COMBINED_SHADER_STORAGE_BUFFERS
> >>> (MAX_SHADER_STORAGE_BUFFERS * 6)
> >>> #define MAX_ATOMIC_COUNTERS 4096
> >>> /* 6 is for vertex, hull, domain, geometry, fragment, and compute
> >>> shader. */
> >>> #define MAX_COMBINED_ATOMIC_BUFFERS (MAX_UNIFORM_BUFFERS * 6)
> >>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> >>> index 8a59b5e..8577e43 100644
> >>> --- a/src/mesa/main/context.c
> >>> +++ b/src/mesa/main/context.c
> >>> @@ -615,6 +615,12 @@ _mesa_init_constants(struct gl_constants *consts,
> >>> gl_api api)
> >>> consts->MaxUniformBlockSize = 16384;
> >>> consts->UniformBufferOffsetAlignment = 1;
> >>>
> >>> + /** GL_ARB_shader_storage_buffer_object */
> >>> + consts->MaxCombinedShaderStorageBlocks = 36;
> >>
> >> Spec has 8. I guess 36 > 8, so this may be okay, but where is 36 from?
> >
> > As explained before, we picked it from uniforms:
> > consts->MaxCombinedUniformBlocks = 36;
> >
> >>
> >>> + consts->MaxShaderStorageBufferBindings = 36;
> >>
> >> Spec has 8. Same comment/question as above.
> >
> > Copied from consts->MaxUniformBufferBindings = 36;
> >>
> >>> + consts->MaxShaderStorageBlockSize = 16 * 1024 * 1024;
> >>> + consts->ShaderStorageBufferOffsetAlignment = 1;
> >>
> >> Spec has 256.
> >>
> >
> > Oh, right. We picked it from consts->UniformBufferOffsetAlignment = 1
> > but, according to the spec, this is a mistake. We will fix it!
> >
> > So, just to confirm, we will pick the spec values (if available) for all
> > cases. If you (or somebody else) think a better default value should be
> > chosen for any constant, please say so :-)
> >
>
> By the way, next patch "mesa: add MaxShaderStorageBlocks to struct
> gl_program_constants" [0] sets prog->MaxShaderStorageBlocks = 12, which
> is also copied from prog->MaxUniformBlocks = 12.
> Because of that, one piglit test [1] starts to fail because
> MaxCombinedUniformBlocks is now set to 8 but we have more shader storage
> block per shader.
>
> I plan to modify patch [0] to set prog->MaxShaderStorageBlocks = 8 and
> write a separate patch that makes i965 driver to set the following
> constant values:
>
> * MaxShaderStorageBlocks = 12
> * MaxCombinedUniformBlocks = 36 (12 * 3 different shader types: vertex,
> geometry and fragment)
Will it need to be '* 5' with the 2 tessellation stages? I wonder if
we can somehow guard against it being missed later. I guess TS tests
should be able to catch this.
Anyway, with the ShaderStorageBufferOffsetAlignment fix:
Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> * MaxShaderStorageBufferBindings = 36.
>
> Those values are arbitrary (picked from uniforms) but our tests seem to
> work with them. As it was said in last email, if you think we should
> change the values or this is not right, just say so.
>
> What do you think?
>
> Sam
>
> [0] http://lists.freedesktop.org/archives/mesa-dev/2015-June/085570.html
> [1] bin/arb_shader_storage_buffer_object-maxblocks -auto -fbo
>
> > Thanks!
> >
> > Sam
> >
> >> -Jordan
> >>
> >>> +
> >>> /* GL_ARB_explicit_uniform_location, GL_MAX_UNIFORM_LOCATIONS */
> >>> consts->MaxUserAssignableUniformLocations =
> >>> 4 * MESA_SHADER_STAGES * MAX_UNIFORMS;
> >>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> >>> index 03b8e48..741930d 100644
> >>> --- a/src/mesa/main/mtypes.h
> >>> +++ b/src/mesa/main/mtypes.h
> >>> @@ -3370,6 +3370,15 @@ struct gl_constants
> >>> GLuint UniformBufferOffsetAlignment;
> >>> /** @} */
> >>>
> >>> + /** @{
> >>> + * GL_ARB_shader_storage_buffer_object
> >>> + */
> >>> + GLuint MaxCombinedShaderStorageBlocks;
> >>> + GLuint MaxShaderStorageBufferBindings;
> >>> + GLuint MaxShaderStorageBlockSize;
> >>> + GLuint ShaderStorageBufferOffsetAlignment;
> >>> + /** @} */
> >>> +
> >>> /**
> >>> * GL_ARB_explicit_uniform_location
> >>> */
> >>> @@ -4015,6 +4024,20 @@ struct gl_uniform_buffer_binding
> >>> GLboolean AutomaticSize;
> >>> };
> >>>
> >>> +struct gl_shader_storage_buffer_binding
> >>> +{
> >>> + struct gl_buffer_object *BufferObject;
> >>> + /** Start of shader storage block data in the buffer */
> >>> + GLintptr Offset;
> >>> + /** Size of data allowed to be referenced from the buffer (in bytes)
> >>> */
> >>> + GLsizeiptr Size;
> >>> + /**
> >>> + * glBindBufferBase() indicates that the Size should be ignored and
> >>> only
> >>> + * limited by the current size of the BufferObject.
> >>> + */
> >>> + GLboolean AutomaticSize;
> >>> +};
> >>> +
> >>> /**
> >>> * ARB_shader_image_load_store image unit.
> >>> */
> >>> @@ -4262,6 +4285,12 @@ struct gl_context
> >>> struct gl_buffer_object *UniformBuffer;
> >>>
> >>> /**
> >>> + * Current GL_ARB_shader_storage_buffer_object binding referenced by
> >>> + * GL_SHADER_STORAGE_BUFFER target for glBufferData, glMapBuffer, etc.
> >>> + */
> >>> + struct gl_buffer_object *ShaderStorageBuffer;
> >>> +
> >>> + /**
> >>> * Array of uniform buffers for GL_ARB_uniform_buffer_object and GL
> >>> 3.1.
> >>> * This is set up using glBindBufferRange() or glBindBufferBase().
> >>> They are
> >>> * associated with uniform blocks by glUniformBlockBinding()'s state
> >>> in the
> >>> @@ -4271,6 +4300,15 @@ struct gl_context
> >>> UniformBufferBindings[MAX_COMBINED_UNIFORM_BUFFERS];
> >>>
> >>> /**
> >>> + * Array of shader storage buffers for
> >>> ARB_shader_storage_buffer_object
> >>> + * and GL 4.3. This is set up using glBindBufferRange() or
> >>> + * glBindBufferBase(). They are associated with shader storage
> >>> blocks by
> >>> + * glShaderStorageBlockBinding()'s state in the shader program.
> >>> + */
> >>> + struct gl_shader_storage_buffer_binding
> >>> + ShaderStorageBufferBindings[MAX_COMBINED_SHADER_STORAGE_BUFFERS];
> >>> +
> >>> + /**
> >>> * Object currently associated with the GL_ATOMIC_COUNTER_BUFFER
> >>> * target.
> >>> */
> >>> --
> >>> 1.9.1
> >>>
> >>> _______________________________________________
> >>> mesa-dev mailing list
> >>> mesa-dev@lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
