[Mesa-dev] [PATCH 03/11] mesa: Add state data structures requried for ARB_shader_image_load_store.

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:

> On 24 November 2013 21:00, Francisco Jerez <currojerez at riseup.net> wrote:
>>[...]
>> @@ -175,6 +175,7 @@
>>  #define MAX_COMBINED_ATOMIC_BUFFERS    (MAX_UNIFORM_BUFFERS * 6)
>>  /* Size of an atomic counter in bytes according to
>> ARB_shader_atomic_counters */
>>  #define ATOMIC_COUNTER_SIZE            4
>> +#define MAX_IMAGE_UNITS                32
>>
>
> It would be nice to have a note in the commit message explaining why you
> chose 32 for MAX_IMAGE_UNITS (the spec only requires 8 so it's not obvious).
>

There's no special reason.  I think I'm going to change it to 96,
because that would match the number of shader stages (when we support
compute and tesselation shaders) times the number of image uniforms that
we're going to expose in i965 (16, which is also completely arbitrary,
I'm open to suggestions), so it will allow binding the maximum number of
independent images to the pipeline on i965 given the per-stage limits.

>
>>  /*@}*/
>>
>>  /**
>> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
>> index b5b874f..648062f 100644
>> --- a/src/mesa/main/dd.h
>> +++ b/src/mesa/main/dd.h
>> @@ -39,6 +39,7 @@ struct gl_buffer_object;
>>  struct gl_context;
>>  struct gl_display_list;
>>  struct gl_framebuffer;
>> +struct gl_image_unit;
>>  struct gl_pixelstore_attrib;
>>  struct gl_program;
>>  struct gl_renderbuffer;
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index e9750f4..7be0664 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -1207,6 +1207,9 @@ struct gl_texture_object
>>
>>     /** GL_OES_EGL_image_external */
>>     GLint RequiredTextureImageUnits;
>> +
>> +   /** GL_ARB_shader_image_load_store */
>> +   GLenum ImageFormatCompatibility;
>>
>
> This is called IMAGE_FORMAT_COMPATIBILITY_TYPE in the GL spec.  Can we
> rename it to ImageFormatCompatibilityType for consistency?
>

Meh...  That will result in some lines going over the 80-line limit, but
I guess I could wrap them.

>
>>  };
>>
>>
>> @@ -2373,6 +2376,29 @@ struct gl_shader
>>          */
>>        GLenum OutputType;
>>     } Geom;
>> +
>> +   /**
>> +    * Map from image uniform index to image unit (set by glUniform1i())
>> +    *
>> +    * An image uniform index is associated with each image uniform by
>> +    * the linker.  The image index associated with each uniform is
>> +    * stored in the \c gl_uniform_storage::image field.
>>
>
> Can we assume image uniform indices are always in the range [0,
> NumImages-1]?  If so it would be nice to document that here.
>
>
>> +    */
>> +   GLubyte ImageUnits[MAX_IMAGE_UNITS];
>> +
>> +   /**
>> +    * Access qualifier specified in the shader for each image uniform.
>> +    * Either \c GL_READ_ONLY, \c GL_WRITE_ONLY or \c GL_READ_WRITE.
>> +    *
>> +    * It may be different, though only more strict than the value of
>> +    * \c gl_image_unit::Access for the corresponding image unit.
>>
>
> Is this array indexed by image uniform index or by image unit?  The comment
> should clarify that.
>
>
>> +    */
>> +   GLenum ImageAccess[MAX_IMAGE_UNITS];
>>
>
> With those changes, this patch is:
>
> Reviewed-by: Paul Berry <stereotype441 at gmail.com>

I think I've taken into account all your comments here [1], and I've
changed the value of MAX_IMAGE_UNITS.  Let me know if your Reviewed-by
still applies.

[1] http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=f281b85e4c6b20ac8b1653313a8276e76b8b285e
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131208/2289abe3/attachment.pgp>