[Mesa-dev,RFC] gallium: add interface for writable shader images

Submitted by Marek Olšák on July 5, 2015, 1:25 p.m.

Details

Message ID 1436102749-31748-1-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák July 5, 2015, 1:25 p.m.
From: Marek Olšák <marek.olsak@amd.com>

Other approaches are being considered:

1) Don't use resource wrappers (views) and pass all view parameters
   (format, layer range, level) to set_shader_images just like
   set_vertex_buffers, set_constant_buffer, or even glBindImageTexture do.

2) Use pipe_sampler_view instead of pipe_image_view,
   and maybe even use set_sampler_views instead of set_shader_images.
   set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS for
   all writable images to allow for OpenGL textures in the lower slots.

I wouldn't like to go back to using pipe_surface, because it doesn't fit
into the DX12 hw design.

Please discuss.
---
 src/gallium/auxiliary/util/u_debug_describe.c |  9 +++++++
 src/gallium/auxiliary/util/u_debug_describe.h |  2 ++
 src/gallium/auxiliary/util/u_dump.h           |  3 +++
 src/gallium/auxiliary/util/u_dump_state.c     | 27 +++++++++++++++++++++
 src/gallium/auxiliary/util/u_inlines.h        | 10 ++++++++
 src/gallium/docs/source/context.rst           | 16 ++++++------
 src/gallium/drivers/hangdump/hd_context.c     |  3 ++-
 src/gallium/drivers/ilo/ilo_state.c           | 10 +++++---
 src/gallium/drivers/nouveau/nvc0/nvc0_state.c | 10 +++++---
 src/gallium/include/pipe/p_context.h          | 35 ++++++++++++++++++---------
 src/gallium/include/pipe/p_state.h            | 25 +++++++++++++++++++
 11 files changed, 122 insertions(+), 28 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/auxiliary/util/u_debug_describe.c b/src/gallium/auxiliary/util/u_debug_describe.c
index df73ed8..f428d22 100644
--- a/src/gallium/auxiliary/util/u_debug_describe.c
+++ b/src/gallium/auxiliary/util/u_debug_describe.c
@@ -81,6 +81,15 @@  debug_describe_sampler_view(char* buf, const struct pipe_sampler_view *ptr)
 }
 
 void
+debug_describe_image_view(char* buf, const struct pipe_image_view *ptr)
+{
+   char res[128];
+   debug_describe_resource(res, ptr->resource);
+   util_sprintf(buf, "pipe_image_view<%s,%s>", res,
+                util_format_short_name(ptr->format));
+}
+
+void
 debug_describe_so_target(char* buf,
                          const struct pipe_stream_output_target *ptr)
 {
diff --git a/src/gallium/auxiliary/util/u_debug_describe.h b/src/gallium/auxiliary/util/u_debug_describe.h
index 4f7882b..2172ecb 100644
--- a/src/gallium/auxiliary/util/u_debug_describe.h
+++ b/src/gallium/auxiliary/util/u_debug_describe.h
@@ -35,12 +35,14 @@  struct pipe_reference;
 struct pipe_resource;
 struct pipe_surface;
 struct pipe_sampler_view;
+struct pipe_image_view;
 
 /* a 256-byte buffer is necessary and sufficient */
 void debug_describe_reference(char* buf, const struct pipe_reference*ptr);
 void debug_describe_resource(char* buf, const struct pipe_resource *ptr);
 void debug_describe_surface(char* buf, const struct pipe_surface *ptr);
 void debug_describe_sampler_view(char* buf, const struct pipe_sampler_view *ptr);
+void debug_describe_image_view(char* buf, const struct pipe_image_view *ptr);
 void debug_describe_so_target(char* buf,
                               const struct pipe_stream_output_target *ptr);
 
diff --git a/src/gallium/auxiliary/util/u_dump.h b/src/gallium/auxiliary/util/u_dump.h
index 1d279ef..4304613 100644
--- a/src/gallium/auxiliary/util/u_dump.h
+++ b/src/gallium/auxiliary/util/u_dump.h
@@ -156,6 +156,9 @@  void
 util_dump_sampler_view(FILE *stream, const struct pipe_sampler_view *state);
 
 void
+util_dump_image_view(FILE *stream, const struct pipe_image_view *state);
+
+void
 util_dump_transfer(FILE *stream,
                    const struct pipe_transfer *state);
 
diff --git a/src/gallium/auxiliary/util/u_dump_state.c b/src/gallium/auxiliary/util/u_dump_state.c
index 7784eb9..be0b1b2 100644
--- a/src/gallium/auxiliary/util/u_dump_state.c
+++ b/src/gallium/auxiliary/util/u_dump_state.c
@@ -708,6 +708,33 @@  util_dump_sampler_view(FILE *stream, const struct pipe_sampler_view *state)
 
 
 void
+util_dump_image_view(FILE *stream, const struct pipe_image_view *state)
+{
+   if (!state) {
+      util_dump_null(stream);
+      return;
+   }
+
+   util_dump_struct_begin(stream, "pipe_image_view");
+
+   util_dump_member(stream, ptr, state, resource);
+   util_dump_member(stream, format, state, format);
+
+   if (state->resource->target == PIPE_BUFFER) {
+      util_dump_member(stream, uint, state, u.buf.first_element);
+      util_dump_member(stream, uint, state, u.buf.last_element);
+   }
+   else {
+      util_dump_member(stream, uint, state, u.tex.first_layer);
+      util_dump_member(stream, uint, state, u.tex.last_layer);
+      util_dump_member(stream, uint, state, u.tex.level);
+   }
+
+   util_dump_struct_end(stream);
+}
+
+
+void
 util_dump_transfer(FILE *stream, const struct pipe_transfer *state)
 {
    if(!state) {
diff --git a/src/gallium/auxiliary/util/u_inlines.h b/src/gallium/auxiliary/util/u_inlines.h
index 9540162..661a949 100644
--- a/src/gallium/auxiliary/util/u_inlines.h
+++ b/src/gallium/auxiliary/util/u_inlines.h
@@ -173,6 +173,16 @@  pipe_sampler_view_release(struct pipe_context *ctx,
    *ptr = NULL;
 }
 
+static INLINE void
+pipe_image_view_reference(struct pipe_image_view **ptr, struct pipe_image_view *view)
+{
+   struct pipe_image_view *old_view = *ptr;
+
+   if (pipe_reference_described(&(*ptr)->reference, &view->reference,
+                                (debug_reference_descriptor)debug_describe_image_view))
+      old_view->context->image_view_destroy(old_view->context, old_view);
+   *ptr = view;
+}
 
 static INLINE void
 pipe_so_target_reference(struct pipe_stream_output_target **ptr,
diff --git a/src/gallium/docs/source/context.rst b/src/gallium/docs/source/context.rst
index 0908ee7..a7d08d2 100644
--- a/src/gallium/docs/source/context.rst
+++ b/src/gallium/docs/source/context.rst
@@ -131,14 +131,14 @@  from a shader without an associated sampler.  This means that they
 have no support for floating point coordinates, address wrap modes or
 filtering.
 
-Shader resources are specified for all the shader stages at once using
-the ``set_shader_resources`` method.  When binding texture resources,
-the ``level``, ``first_layer`` and ``last_layer`` pipe_surface fields
-specify the mipmap level and the range of layers the texture will be
-constrained to.  In the case of buffers, ``first_element`` and
-``last_element`` specify the range within the buffer that will be used
-by the shader resource.  Writes to a shader resource are only allowed
-when the ``writable`` flag is set.
+There are 2 types of shader resources: buffers and images.
+
+Buffers are specified using the ``set_shader_buffers`` method.
+
+Images are specified using the ``set_shader_images`` method. When binding
+images, the ``level``, ``first_layer`` and ``last_layer`` pipe_image_view
+fields specify the mipmap level and the range of layers the image will be
+constrained to.
 
 Surfaces
 ^^^^^^^^
diff --git a/src/gallium/drivers/hangdump/hd_context.c b/src/gallium/drivers/hangdump/hd_context.c
index 0563415..80f2f22 100644
--- a/src/gallium/drivers/hangdump/hd_context.c
+++ b/src/gallium/drivers/hangdump/hd_context.c
@@ -732,7 +732,8 @@  hd_context_create(struct hd_screen *hscreen, struct pipe_context *pipe)
    CTX_INIT(set_viewport_states);
    CTX_INIT(set_sampler_views);
    CTX_INIT(set_tess_state);
-   /* set_shader_resources */
+   /* set_shader_buffers */
+   /* set_shader_images */
    CTX_INIT(set_vertex_buffers);
    CTX_INIT(set_index_buffer);
    CTX_INIT(create_stream_output_target);
diff --git a/src/gallium/drivers/ilo/ilo_state.c b/src/gallium/drivers/ilo/ilo_state.c
index b1bd49a..8076855 100644
--- a/src/gallium/drivers/ilo/ilo_state.c
+++ b/src/gallium/drivers/ilo/ilo_state.c
@@ -844,10 +844,11 @@  ilo_set_sampler_views(struct pipe_context *pipe, unsigned shader,
 }
 
 static void
-ilo_set_shader_resources(struct pipe_context *pipe,
-                         unsigned start, unsigned count,
-                         struct pipe_surface **surfaces)
+ilo_set_shader_images(struct pipe_context *pipe,
+                      unsigned start, unsigned count,
+                      struct pipe_image_view **views)
 {
+#if 0
    struct ilo_state_vector *vec = &ilo_context(pipe)->state_vector;
    struct ilo_resource_state *dst = &vec->resource;
    unsigned i;
@@ -876,6 +877,7 @@  ilo_set_shader_resources(struct pipe_context *pipe,
    }
 
    vec->dirty |= ILO_DIRTY_RESOURCE;
+#endif
 }
 
 static void
@@ -1269,7 +1271,7 @@  ilo_init_state_functions(struct ilo_context *ilo)
    ilo->base.set_scissor_states = ilo_set_scissor_states;
    ilo->base.set_viewport_states = ilo_set_viewport_states;
    ilo->base.set_sampler_views = ilo_set_sampler_views;
-   ilo->base.set_shader_resources = ilo_set_shader_resources;
+   ilo->base.set_shader_images = ilo_set_shader_images;
    ilo->base.set_vertex_buffers = ilo_set_vertex_buffers;
    ilo->base.set_index_buffer = ilo_set_index_buffer;
 
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
index 63c3c52..490ca9f 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
@@ -1125,13 +1125,15 @@  nvc0_set_compute_resources(struct pipe_context *pipe,
 }
 
 static void
-nvc0_set_shader_resources(struct pipe_context *pipe,
+nvc0_set_shader_images(struct pipe_context *pipe,
                           unsigned start, unsigned nr,
-                          struct pipe_surface **resources)
+                          struct pipe_image_view **views)
 {
-   nvc0_bind_surfaces_range(nvc0_context(pipe), 0, start, nr, resources);
+#if 0
+   nvc0_bind_surfaces_range(nvc0_context(pipe), 0, start, nr, views);
 
    nvc0_context(pipe)->dirty |= NVC0_NEW_SURFACES;
+#endif
 }
 
 static INLINE void
@@ -1253,7 +1255,7 @@  nvc0_init_state_functions(struct nvc0_context *nvc0)
 
    pipe->set_global_binding = nvc0_set_global_bindings;
    pipe->set_compute_resources = nvc0_set_compute_resources;
-   pipe->set_shader_resources = nvc0_set_shader_resources;
+   pipe->set_shader_images = nvc0_set_shader_images;
 
    nvc0->sample_mask = ~0;
    nvc0->min_samples = 1;
diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
index c2eedf8..13f3938 100644
--- a/src/gallium/include/pipe/p_context.h
+++ b/src/gallium/include/pipe/p_context.h
@@ -48,6 +48,7 @@  struct pipe_depth_stencil_alpha_state;
 struct pipe_draw_info;
 struct pipe_fence_handle;
 struct pipe_framebuffer_state;
+struct pipe_image_view;
 struct pipe_index_buffer;
 struct pipe_query;
 struct pipe_poly_stipple;
@@ -236,20 +237,21 @@  struct pipe_context {
                           const float default_inner_level[2]);
 
    /**
-    * Bind an array of shader resources that will be used by the
-    * graphics pipeline.  Any resources that were previously bound to
-    * the specified range will be unbound after this call.
+    * Bind an array of images that will be used by a shader.
+    * Any imagse that were previously bound to the specified range
+    * will be unbound.
     *
-    * \param start      first resource to bind.
-    * \param count      number of consecutive resources to bind.
-    * \param resources  array of pointers to the resources to bind, it
+    * \param shader     shader stage where the images will be bound.
+    * \param start_slot first iamage slot to bind.
+    * \param count      number of consecutive images to bind.
+    * \param buffers    array of pointers to the images to bind, it
     *                   should contain at least \a count elements
-    *                   unless it's NULL, in which case no new
-    *                   resources will be bound.
+    *                   unless it's NULL, in which case no images will
+    *                   be bound.
     */
-   void (*set_shader_resources)(struct pipe_context *,
-                                unsigned start, unsigned count,
-                                struct pipe_surface **resources);
+   void (*set_shader_images)(struct pipe_context *, unsigned shader,
+                             unsigned start_slot, unsigned count,
+                             struct pipe_image_view *images);
 
    void (*set_vertex_buffers)( struct pipe_context *,
                                unsigned start_slot,
@@ -392,6 +394,17 @@  struct pipe_context {
                            struct pipe_surface *);
 
    /**
+    * Create an image view into a buffer or texture to be used with load,
+    * store, and atomic instructions by a shader stage.
+    */
+   struct pipe_image_view * (*create_image_view)(struct pipe_context *ctx,
+                                                 struct pipe_resource *texture,
+                                                 const struct pipe_image_view *templat);
+
+   void (*image_view_destroy)(struct pipe_context *ctx,
+                              struct pipe_image_view *view);
+
+   /**
     * Map a resource.
     *
     * Transfers are (by default) context-private and allow uploads to be
diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
index dc8f550..f655dda 100644
--- a/src/gallium/include/pipe/p_state.h
+++ b/src/gallium/include/pipe/p_state.h
@@ -386,6 +386,31 @@  struct pipe_sampler_view
 
 
 /**
+ * A view into a writable buffer or texture that can be bound to a shader
+ * stage.
+ */
+struct pipe_image_view
+{
+   struct pipe_reference reference;
+   struct pipe_resource *resource; /**< resource into which this is a view  */
+   struct pipe_context *context; /**< context this view belongs to */
+   enum pipe_format format;      /**< typed PIPE_FORMAT_x */
+
+   union {
+      struct {
+         unsigned first_layer:16;     /**< first layer to use for array textures */
+         unsigned last_layer:16;      /**< last layer to use for array textures */
+         unsigned level:8;            /**< mipmap level to use */
+      } tex;
+      struct {
+         unsigned first_element;
+         unsigned last_element;
+      } buf;
+   } u;
+};
+
+
+/**
  * Subregion of 1D/2D/3D image resource.
  */
 struct pipe_box

Comments

On Sun, Jul 5, 2015 at 9:25 AM, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> Other approaches are being considered:
>
> 1) Don't use resource wrappers (views) and pass all view parameters
>    (format, layer range, level) to set_shader_images just like
>    set_vertex_buffers, set_constant_buffer, or even glBindImageTexture do.
>
> 2) Use pipe_sampler_view instead of pipe_image_view,
>    and maybe even use set_sampler_views instead of set_shader_images.
>    set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS for
>    all writable images to allow for OpenGL textures in the lower slots.
>
> I wouldn't like to go back to using pipe_surface, because it doesn't fit
> into the DX12 hw design.
>
> Please discuss.
> ---
> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
> index c2eedf8..13f3938 100644
> --- a/src/gallium/include/pipe/p_context.h
> +++ b/src/gallium/include/pipe/p_context.h
> @@ -48,6 +48,7 @@ struct pipe_depth_stencil_alpha_state;
>  struct pipe_draw_info;
>  struct pipe_fence_handle;
>  struct pipe_framebuffer_state;
> +struct pipe_image_view;
>  struct pipe_index_buffer;
>  struct pipe_query;
>  struct pipe_poly_stipple;
> @@ -236,20 +237,21 @@ struct pipe_context {
>                            const float default_inner_level[2]);
>
>     /**
> -    * Bind an array of shader resources that will be used by the
> -    * graphics pipeline.  Any resources that were previously bound to
> -    * the specified range will be unbound after this call.
> +    * Bind an array of images that will be used by a shader.
> +    * Any imagse that were previously bound to the specified range
> +    * will be unbound.
>      *
> -    * \param start      first resource to bind.
> -    * \param count      number of consecutive resources to bind.
> -    * \param resources  array of pointers to the resources to bind, it
> +    * \param shader     shader stage where the images will be bound.
> +    * \param start_slot first iamage slot to bind.
> +    * \param count      number of consecutive images to bind.
> +    * \param buffers    array of pointers to the images to bind, it
>      *                   should contain at least \a count elements
> -    *                   unless it's NULL, in which case no new
> -    *                   resources will be bound.
> +    *                   unless it's NULL, in which case no images will
> +    *                   be bound.
>      */
> -   void (*set_shader_resources)(struct pipe_context *,
> -                                unsigned start, unsigned count,
> -                                struct pipe_surface **resources);
> +   void (*set_shader_images)(struct pipe_context *, unsigned shader,
> +                             unsigned start_slot, unsigned count,
> +                             struct pipe_image_view *images);

On Fermi, there is only one set of 8 images that can be bound,
irrespective of stage. There are a different set of 8 bindings for 3d
and compute pipelines though. Kepler+ is all bindless so it can do
whatever.

Even though I originally advocated for the stage argument, it appears
to neither match Fermi hardware nor the GL API where I believe images
are also in a global namespace.

>
>     void (*set_vertex_buffers)( struct pipe_context *,
>                                 unsigned start_slot,
> @@ -392,6 +394,17 @@ struct pipe_context {
>                             struct pipe_surface *);
>
>     /**
> +    * Create an image view into a buffer or texture to be used with load,
> +    * store, and atomic instructions by a shader stage.
> +    */
> +   struct pipe_image_view * (*create_image_view)(struct pipe_context *ctx,
> +                                                 struct pipe_resource *texture,
> +                                                 const struct pipe_image_view *templat);
> +
> +   void (*image_view_destroy)(struct pipe_context *ctx,
> +                              struct pipe_image_view *view);
> +
> +   /**
>      * Map a resource.
>      *
>      * Transfers are (by default) context-private and allow uploads to be
> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
> index dc8f550..f655dda 100644
> --- a/src/gallium/include/pipe/p_state.h
> +++ b/src/gallium/include/pipe/p_state.h
> @@ -386,6 +386,31 @@ struct pipe_sampler_view
>
>
>  /**
> + * A view into a writable buffer or texture that can be bound to a shader
> + * stage.
> + */
> +struct pipe_image_view
> +{
> +   struct pipe_reference reference;
> +   struct pipe_resource *resource; /**< resource into which this is a view  */
> +   struct pipe_context *context; /**< context this view belongs to */
> +   enum pipe_format format;      /**< typed PIPE_FORMAT_x */
> +
> +   union {
> +      struct {
> +         unsigned first_layer:16;     /**< first layer to use for array textures */
> +         unsigned last_layer:16;      /**< last layer to use for array textures */
> +         unsigned level:8;            /**< mipmap level to use */
> +      } tex;
> +      struct {
> +         unsigned first_element;
> +         unsigned last_element;
> +      } buf;
> +   } u;
> +};

This is an exact copy of pipe_surface, except for the
supposedly-removable width/height and "writable" bit, which should
probably make a comeback here.

Can you remind me what was wrong with pipe_surface? I seem to recall
that you were offended by its refcountedness, but that appears to have
stayed in this version.

  -ilia
Am 05.07.2015 um 15:47 schrieb Ilia Mirkin:
> On Sun, Jul 5, 2015 at 9:25 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> Other approaches are being considered:
>>
>> 1) Don't use resource wrappers (views) and pass all view parameters
>>    (format, layer range, level) to set_shader_images just like
>>    set_vertex_buffers, set_constant_buffer, or even glBindImageTexture do.
>>
>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>    and maybe even use set_sampler_views instead of set_shader_images.
>>    set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS for
>>    all writable images to allow for OpenGL textures in the lower slots.
>>
>> I wouldn't like to go back to using pipe_surface, because it doesn't fit
>> into the DX12 hw design.
>>
>> Please discuss.
>> ---
>> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>> index c2eedf8..13f3938 100644
>> --- a/src/gallium/include/pipe/p_context.h
>> +++ b/src/gallium/include/pipe/p_context.h
>> @@ -48,6 +48,7 @@ struct pipe_depth_stencil_alpha_state;
>>  struct pipe_draw_info;
>>  struct pipe_fence_handle;
>>  struct pipe_framebuffer_state;
>> +struct pipe_image_view;
>>  struct pipe_index_buffer;
>>  struct pipe_query;
>>  struct pipe_poly_stipple;
>> @@ -236,20 +237,21 @@ struct pipe_context {
>>                            const float default_inner_level[2]);
>>
>>     /**
>> -    * Bind an array of shader resources that will be used by the
>> -    * graphics pipeline.  Any resources that were previously bound to
>> -    * the specified range will be unbound after this call.
>> +    * Bind an array of images that will be used by a shader.
>> +    * Any imagse that were previously bound to the specified range
>> +    * will be unbound.
>>      *
>> -    * \param start      first resource to bind.
>> -    * \param count      number of consecutive resources to bind.
>> -    * \param resources  array of pointers to the resources to bind, it
>> +    * \param shader     shader stage where the images will be bound.
>> +    * \param start_slot first iamage slot to bind.
>> +    * \param count      number of consecutive images to bind.
>> +    * \param buffers    array of pointers to the images to bind, it
>>      *                   should contain at least \a count elements
>> -    *                   unless it's NULL, in which case no new
>> -    *                   resources will be bound.
>> +    *                   unless it's NULL, in which case no images will
>> +    *                   be bound.
>>      */
>> -   void (*set_shader_resources)(struct pipe_context *,
>> -                                unsigned start, unsigned count,
>> -                                struct pipe_surface **resources);
>> +   void (*set_shader_images)(struct pipe_context *, unsigned shader,
>> +                             unsigned start_slot, unsigned count,
>> +                             struct pipe_image_view *images);
> 
> On Fermi, there is only one set of 8 images that can be bound,
> irrespective of stage. There are a different set of 8 bindings for 3d
> and compute pipelines though. Kepler+ is all bindless so it can do
> whatever.
> 
> Even though I originally advocated for the stage argument, it appears
> to neither match Fermi hardware nor the GL API where I believe images
> are also in a global namespace.

That looks exactly like d3d11 restrictions. UAVs were only in pixel
shaders and compute shaders. d3d11.1 made them available to all shader
stages though, but they continue to share the same bind points in all
shader stages (and still share them with render targets).
I don't really know though what's the better design. GL though seems to
think this is per shader stage (there are both per-stage and combined
limits just like as for textures).

> 
>>
>>     void (*set_vertex_buffers)( struct pipe_context *,
>>                                 unsigned start_slot,
>> @@ -392,6 +394,17 @@ struct pipe_context {
>>                             struct pipe_surface *);
>>
>>     /**
>> +    * Create an image view into a buffer or texture to be used with load,
>> +    * store, and atomic instructions by a shader stage.
>> +    */
>> +   struct pipe_image_view * (*create_image_view)(struct pipe_context *ctx,
>> +                                                 struct pipe_resource *texture,
>> +                                                 const struct pipe_image_view *templat);
>> +
>> +   void (*image_view_destroy)(struct pipe_context *ctx,
>> +                              struct pipe_image_view *view);
>> +
>> +   /**
>>      * Map a resource.
>>      *
>>      * Transfers are (by default) context-private and allow uploads to be
>> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
>> index dc8f550..f655dda 100644
>> --- a/src/gallium/include/pipe/p_state.h
>> +++ b/src/gallium/include/pipe/p_state.h
>> @@ -386,6 +386,31 @@ struct pipe_sampler_view
>>
>>
>>  /**
>> + * A view into a writable buffer or texture that can be bound to a shader
>> + * stage.
>> + */
>> +struct pipe_image_view
>> +{
>> +   struct pipe_reference reference;
>> +   struct pipe_resource *resource; /**< resource into which this is a view  */
>> +   struct pipe_context *context; /**< context this view belongs to */
>> +   enum pipe_format format;      /**< typed PIPE_FORMAT_x */
>> +
>> +   union {
>> +      struct {
>> +         unsigned first_layer:16;     /**< first layer to use for array textures */
>> +         unsigned last_layer:16;      /**< last layer to use for array textures */
>> +         unsigned level:8;            /**< mipmap level to use */
>> +      } tex;
>> +      struct {
>> +         unsigned first_element;
>> +         unsigned last_element;
>> +      } buf;
>> +   } u;
>> +};
> 
> This is an exact copy of pipe_surface, except for the
> supposedly-removable width/height and "writable" bit, which should
> probably make a comeback here.
For d3d11 at the device level, UAVs can have a couple of flags, though
for buffers only
(https://msdn.microsoft.com/en-us/library/windows/hardware/ff542033%28v=vs.85%29.aspx).
Writable isn't one of them though, but if it's useful why not. I always
get confused by this, but I think you've got ro and rw structured
buffers, raw buffers, or "normal" uav textures/buffers at the shader
level in d3d11, but the UAVs are going to be the same. I could be wrong
though...

> 
> Can you remind me what was wrong with pipe_surface? I seem to recall
> that you were offended by its refcountedness, but that appears to have
> stayed in this version.
> 

I think this wasn't terribly consistent. A RT/DS (which is what
pipe_surface is for) is not the same as a UAV (image view), so using a
different entity makes this more clear, even though it's mostly the same
structure. In particular, a different function for creating it rather
than pipe_create_surface should be used (or have some other means to
distinguish it, but another function is probably easiest).

Overall this looks quite reasonable to me, but I am not too familiar
with it.

Roland
On Sun, Jul 5, 2015 at 3:47 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Sun, Jul 5, 2015 at 9:25 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> Other approaches are being considered:
>>
>> 1) Don't use resource wrappers (views) and pass all view parameters
>>    (format, layer range, level) to set_shader_images just like
>>    set_vertex_buffers, set_constant_buffer, or even glBindImageTexture do.
>>
>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>    and maybe even use set_sampler_views instead of set_shader_images.
>>    set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS for
>>    all writable images to allow for OpenGL textures in the lower slots.
>>
>> I wouldn't like to go back to using pipe_surface, because it doesn't fit
>> into the DX12 hw design.
>>
>> Please discuss.
>> ---
>> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>> index c2eedf8..13f3938 100644
>> --- a/src/gallium/include/pipe/p_context.h
>> +++ b/src/gallium/include/pipe/p_context.h
>> @@ -48,6 +48,7 @@ struct pipe_depth_stencil_alpha_state;
>>  struct pipe_draw_info;
>>  struct pipe_fence_handle;
>>  struct pipe_framebuffer_state;
>> +struct pipe_image_view;
>>  struct pipe_index_buffer;
>>  struct pipe_query;
>>  struct pipe_poly_stipple;
>> @@ -236,20 +237,21 @@ struct pipe_context {
>>                            const float default_inner_level[2]);
>>
>>     /**
>> -    * Bind an array of shader resources that will be used by the
>> -    * graphics pipeline.  Any resources that were previously bound to
>> -    * the specified range will be unbound after this call.
>> +    * Bind an array of images that will be used by a shader.
>> +    * Any imagse that were previously bound to the specified range
>> +    * will be unbound.
>>      *
>> -    * \param start      first resource to bind.
>> -    * \param count      number of consecutive resources to bind.
>> -    * \param resources  array of pointers to the resources to bind, it
>> +    * \param shader     shader stage where the images will be bound.
>> +    * \param start_slot first iamage slot to bind.
>> +    * \param count      number of consecutive images to bind.
>> +    * \param buffers    array of pointers to the images to bind, it
>>      *                   should contain at least \a count elements
>> -    *                   unless it's NULL, in which case no new
>> -    *                   resources will be bound.
>> +    *                   unless it's NULL, in which case no images will
>> +    *                   be bound.
>>      */
>> -   void (*set_shader_resources)(struct pipe_context *,
>> -                                unsigned start, unsigned count,
>> -                                struct pipe_surface **resources);
>> +   void (*set_shader_images)(struct pipe_context *, unsigned shader,
>> +                             unsigned start_slot, unsigned count,
>> +                             struct pipe_image_view *images);
>
> On Fermi, there is only one set of 8 images that can be bound,
> irrespective of stage. There are a different set of 8 bindings for 3d
> and compute pipelines though. Kepler+ is all bindless so it can do
> whatever.

Evergreen only has 12 images shared by fragment and compute. Other
shaders can't access them (I think). That's old DX11 hardware though.
SI+ is all bindless too.

>
> Even though I originally advocated for the stage argument, it appears
> to neither match Fermi hardware nor the GL API where I believe images
> are also in a global namespace.

Textures are in a global namespace too, but that doesn't make them
global in shaders. It really depends on whether
combined=vertex=fragment=... or if combined=vertex+fragment+.... Now
that you brought this up, I've checked what Catalyst reports and
indeed it's the former for all SSBOs, images, and atomics. For
simplicity and less needed space for binding slots and less overhead
managing them, we should follow that design too. So let's remove the
"shader" argument from set_shader_images and set_shader_buffers.

>
>>
>>     void (*set_vertex_buffers)( struct pipe_context *,
>>                                 unsigned start_slot,
>> @@ -392,6 +394,17 @@ struct pipe_context {
>>                             struct pipe_surface *);
>>
>>     /**
>> +    * Create an image view into a buffer or texture to be used with load,
>> +    * store, and atomic instructions by a shader stage.
>> +    */
>> +   struct pipe_image_view * (*create_image_view)(struct pipe_context *ctx,
>> +                                                 struct pipe_resource *texture,
>> +                                                 const struct pipe_image_view *templat);
>> +
>> +   void (*image_view_destroy)(struct pipe_context *ctx,
>> +                              struct pipe_image_view *view);
>> +
>> +   /**
>>      * Map a resource.
>>      *
>>      * Transfers are (by default) context-private and allow uploads to be
>> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
>> index dc8f550..f655dda 100644
>> --- a/src/gallium/include/pipe/p_state.h
>> +++ b/src/gallium/include/pipe/p_state.h
>> @@ -386,6 +386,31 @@ struct pipe_sampler_view
>>
>>
>>  /**
>> + * A view into a writable buffer or texture that can be bound to a shader
>> + * stage.
>> + */
>> +struct pipe_image_view
>> +{
>> +   struct pipe_reference reference;
>> +   struct pipe_resource *resource; /**< resource into which this is a view  */
>> +   struct pipe_context *context; /**< context this view belongs to */
>> +   enum pipe_format format;      /**< typed PIPE_FORMAT_x */
>> +
>> +   union {
>> +      struct {
>> +         unsigned first_layer:16;     /**< first layer to use for array textures */
>> +         unsigned last_layer:16;      /**< last layer to use for array textures */
>> +         unsigned level:8;            /**< mipmap level to use */
>> +      } tex;
>> +      struct {
>> +         unsigned first_element;
>> +         unsigned last_element;
>> +      } buf;
>> +   } u;
>> +};
>
> This is an exact copy of pipe_surface, except for the
> supposedly-removable width/height and "writable" bit, which should
> probably make a comeback here.

Images are always writable. I don't see a point for the flag.

>
> Can you remind me what was wrong with pipe_surface? I seem to recall
> that you were offended by its refcountedness, but that appears to have
> stayed in this version.

pipe_surface is for framebuffers, not shader resources. If I had to
choose between pipe_surface and pipe_sampler_view for this, I would
choose pipe_sampler_view. This is what I proposed in alternative
solution 2.

The problem I had with view/surface wrappers was related to shader R/W
buffers. Vertex buffers and constant buffers don't have views either.
I proposed the same approach for images in alternative solution 1. I
think going entirely view-less is a good idea if there's little chance
in reusing those allocated views.

Marek
I'm not experienced with the semantics around resources that can be 
read/written by shaders, so I can't really make educated comments.

But overall this looks good to me FWIW.

On 05/07/15 14:25, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> Other approaches are being considered:
>
> 1) Don't use resource wrappers (views) and pass all view parameters
>     (format, layer range, level) to set_shader_images just like
>     set_vertex_buffers, set_constant_buffer, or even glBindImageTexture do.

I don't know how much pipe drivers leverage this nowadays, but these 
structures are convenient placeholders for driver data, particular when 
they don't support something (e.g., a certain format, or need some 
swizzling), natively.

>
> 2) Use pipe_sampler_view instead of pipe_image_view,
>     and maybe even use set_sampler_views instead of set_shader_images.
>     set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS for
>     all writable images to allow for OpenGL textures in the lower slots.

If pipe_sampler_view  and pipe_image_view are the same, we could indeed 
use one structure for both.  While still keeping the separate 
create/bind/destroy functions.

This would enable drivers to treat them uniformly internally if they 
wanted (e.g, by concatenating all views bindings into a single array as 
you described). Or seperate internal objects if they wanted.

This seems the best of both worlds.

There is even a precendent: {create,bind,delete}_{fs,vs,gs}_state. 
These all use the same template structure, but drivers are free to 
create joint or disjoint private structures for each kind.  And in face 
llvmpipe (and all draw based drivers), end up using different private 
objects.

Jose
On Tue, Jul 7, 2015 at 4:24 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
> I'm not experienced with the semantics around resources that can be
> read/written by shaders, so I can't really make educated comments.
>
> But overall this looks good to me FWIW.
>
> On 05/07/15 14:25, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> Other approaches are being considered:
>>
>> 1) Don't use resource wrappers (views) and pass all view parameters
>>     (format, layer range, level) to set_shader_images just like
>>     set_vertex_buffers, set_constant_buffer, or even glBindImageTexture
>> do.
>
>
> I don't know how much pipe drivers leverage this nowadays, but these
> structures are convenient placeholders for driver data, particular when they
> don't support something (e.g., a certain format, or need some swizzling),
> natively.
>
>>
>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>     and maybe even use set_sampler_views instead of set_shader_images.
>>     set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS
>> for
>>     all writable images to allow for OpenGL textures in the lower slots.
>
>
> If pipe_sampler_view  and pipe_image_view are the same, we could indeed use
> one structure for both.  While still keeping the separate
> create/bind/destroy functions.

The big difference is that a sampler view has a first/last layer and
first/last level, while image views are more like surfaces which just
have the one of each. But they also need a byte range for buffer
images.

Of course we could just ignore that and guarantee that first==last for images.

>
> This would enable drivers to treat them uniformly internally if they wanted
> (e.g, by concatenating all views bindings into a single array as you
> described). Or seperate internal objects if they wanted.
>
> This seems the best of both worlds.
>
> There is even a precendent: {create,bind,delete}_{fs,vs,gs}_state. These all
> use the same template structure, but drivers are free to create joint or
> disjoint private structures for each kind.  And in face llvmpipe (and all
> draw based drivers), end up using different private objects.
>
> Jose
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 07/07/15 21:28, Ilia Mirkin wrote:
> On Tue, Jul 7, 2015 at 4:24 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
>> I'm not experienced with the semantics around resources that can be
>> read/written by shaders, so I can't really make educated comments.
>>
>> But overall this looks good to me FWIW.
>>
>> On 05/07/15 14:25, Marek Olšák wrote:
>>>
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> Other approaches are being considered:
>>>
>>> 1) Don't use resource wrappers (views) and pass all view parameters
>>>      (format, layer range, level) to set_shader_images just like
>>>      set_vertex_buffers, set_constant_buffer, or even glBindImageTexture
>>> do.
>>
>>
>> I don't know how much pipe drivers leverage this nowadays, but these
>> structures are convenient placeholders for driver data, particular when they
>> don't support something (e.g., a certain format, or need some swizzling),
>> natively.
>>
>>>
>>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>>      and maybe even use set_sampler_views instead of set_shader_images.
>>>      set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS
>>> for
>>>      all writable images to allow for OpenGL textures in the lower slots.
>>
>>
>> If pipe_sampler_view  and pipe_image_view are the same, we could indeed use
>> one structure for both.  While still keeping the separate
>> create/bind/destroy functions.
>
> The big difference is that a sampler view has a first/last layer and
> first/last level, while image views are more like surfaces which just
> have the one of each. But they also need a byte range for buffer
> images.

D3D11_TEX2D_ARRAY_UAV allows to specify first/last layer 
https://msdn.microsoft.com/en-us/library/windows/desktop/ff476242.aspx , 
so it sounds that once pipe_image_view is updated to handle D3D11, the 
difference would reduce to the absence of last_level

> Of course we could just ignore that and guarantee that first==last for images.

Yes, it might not be a bad idea.

Jose
On Tue, Jul 7, 2015 at 4:35 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
> On 07/07/15 21:28, Ilia Mirkin wrote:
>>
>> On Tue, Jul 7, 2015 at 4:24 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
>>>
>>> I'm not experienced with the semantics around resources that can be
>>> read/written by shaders, so I can't really make educated comments.
>>>
>>> But overall this looks good to me FWIW.
>>>
>>> On 05/07/15 14:25, Marek Olšák wrote:
>>>>
>>>>
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> Other approaches are being considered:
>>>>
>>>> 1) Don't use resource wrappers (views) and pass all view parameters
>>>>      (format, layer range, level) to set_shader_images just like
>>>>      set_vertex_buffers, set_constant_buffer, or even glBindImageTexture
>>>> do.
>>>
>>>
>>>
>>> I don't know how much pipe drivers leverage this nowadays, but these
>>> structures are convenient placeholders for driver data, particular when
>>> they
>>> don't support something (e.g., a certain format, or need some swizzling),
>>> natively.
>>>
>>>>
>>>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>>>      and maybe even use set_sampler_views instead of set_shader_images.
>>>>      set_sampler_views would have to use start_slot >= PIPE_MAX_SAMPLERS
>>>> for
>>>>      all writable images to allow for OpenGL textures in the lower
>>>> slots.
>>>
>>>
>>>
>>> If pipe_sampler_view  and pipe_image_view are the same, we could indeed
>>> use
>>> one structure for both.  While still keeping the separate
>>> create/bind/destroy functions.
>>
>>
>> The big difference is that a sampler view has a first/last layer and
>> first/last level, while image views are more like surfaces which just
>> have the one of each. But they also need a byte range for buffer
>> images.
>
>
> D3D11_TEX2D_ARRAY_UAV allows to specify first/last layer
> https://msdn.microsoft.com/en-us/library/windows/desktop/ff476242.aspx , so
> it sounds that once pipe_image_view is updated to handle D3D11, the
> difference would reduce to the absence of last_level

Erm. Duh. OpenGL needs first/last layer too. And pipe_surface has it
too, so it all works out well :)

>
>> Of course we could just ignore that and guarantee that first==last for
>> images.
>
>
> Yes, it might not be a bad idea.
>
> Jose
Am 07.07.2015 um 22:35 schrieb Jose Fonseca:
> On 07/07/15 21:28, Ilia Mirkin wrote:
>> On Tue, Jul 7, 2015 at 4:24 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
>>> I'm not experienced with the semantics around resources that can be
>>> read/written by shaders, so I can't really make educated comments.
>>>
>>> But overall this looks good to me FWIW.
>>>
>>> On 05/07/15 14:25, Marek Olšák wrote:
>>>>
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> Other approaches are being considered:
>>>>
>>>> 1) Don't use resource wrappers (views) and pass all view parameters
>>>>      (format, layer range, level) to set_shader_images just like
>>>>      set_vertex_buffers, set_constant_buffer, or even
>>>> glBindImageTexture
>>>> do.
>>>
>>>
>>> I don't know how much pipe drivers leverage this nowadays, but these
>>> structures are convenient placeholders for driver data, particular
>>> when they
>>> don't support something (e.g., a certain format, or need some
>>> swizzling),
>>> natively.
>>>
>>>>
>>>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>>>      and maybe even use set_sampler_views instead of set_shader_images.
>>>>      set_sampler_views would have to use start_slot >=
>>>> PIPE_MAX_SAMPLERS
>>>> for
>>>>      all writable images to allow for OpenGL textures in the lower
>>>> slots.
>>>
>>>
>>> If pipe_sampler_view  and pipe_image_view are the same, we could
>>> indeed use
>>> one structure for both.  While still keeping the separate
>>> create/bind/destroy functions.
>>
>> The big difference is that a sampler view has a first/last layer and
>> first/last level, while image views are more like surfaces which just
>> have the one of each. But they also need a byte range for buffer
>> images.
> 
> D3D11_TEX2D_ARRAY_UAV allows to specify first/last layer
> https://msdn.microsoft.com/en-us/library/windows/desktop/ff476242.aspx ,
> so it sounds that once pipe_image_view is updated to handle D3D11, the
> difference would reduce to the absence of last_level
> 
>> Of course we could just ignore that and guarantee that first==last for
>> images.
> 
> Yes, it might not be a bad idea.
> 

You could of course argue then isn't it really more like pipe_surface?
At least in d3d11 clearly they are much closer in concept to rts. The
actual structures are of course mostly the same in gallium, the
differences boil down to pipe_surface having (long obsolete)
width/height parameters and a writable flag, whereas sampler views
instead have swizzling fields (I don't think they'd have any use for
this), support multiple levels (again, not needed for shader images /
uavs), and have a target parameter (in d3d10, rts actually have a target
parameter too, but it is of no practical consequence, hence there was no
need for that in gallium - I'm not sure if it would be required for
shader images / uavs, uavs certainly have such target parameter too but
I'm not sure it matters).
But in any case, I'm pretty impartial to what structure is used, as long
as it is created/destroyed separately.

Roland
I'd like to discuss one more thing that will affect whether image
slots will be global (shared by all shaders) or not.

Which image unit an image uniform uses is not a compile-time thing,
but it's specified later using glUniform1i. That means we need a
per-shader table that maps image uniforms to global image units. One
possible solution is to add this pipe_context function:

void (*set_shader_image_mapping)(
   struct pipe_context *, unsigned shader,
   unsigned start_decl_index, unsigned count,
   unsigned *decl_to_slot_mapping);

This is only required if the shader image slots are global and not
per-shader. (if they are per-shader, st/mesa can reorder the slots for
each shader independently just like it already does for textures and
UBOs) Shader storage buffer objects suffer from the same issue. Atomic
counters don't.

Therefore, image slots must be per-shader (like sampler slots) to
avoid this craziness and keep things simple.

Marek



On Tue, Jul 7, 2015 at 10:49 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> Am 07.07.2015 um 22:35 schrieb Jose Fonseca:
>> On 07/07/15 21:28, Ilia Mirkin wrote:
>>> On Tue, Jul 7, 2015 at 4:24 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
>>>> I'm not experienced with the semantics around resources that can be
>>>> read/written by shaders, so I can't really make educated comments.
>>>>
>>>> But overall this looks good to me FWIW.
>>>>
>>>> On 05/07/15 14:25, Marek Olšák wrote:
>>>>>
>>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>>
>>>>> Other approaches are being considered:
>>>>>
>>>>> 1) Don't use resource wrappers (views) and pass all view parameters
>>>>>      (format, layer range, level) to set_shader_images just like
>>>>>      set_vertex_buffers, set_constant_buffer, or even
>>>>> glBindImageTexture
>>>>> do.
>>>>
>>>>
>>>> I don't know how much pipe drivers leverage this nowadays, but these
>>>> structures are convenient placeholders for driver data, particular
>>>> when they
>>>> don't support something (e.g., a certain format, or need some
>>>> swizzling),
>>>> natively.
>>>>
>>>>>
>>>>> 2) Use pipe_sampler_view instead of pipe_image_view,
>>>>>      and maybe even use set_sampler_views instead of set_shader_images.
>>>>>      set_sampler_views would have to use start_slot >=
>>>>> PIPE_MAX_SAMPLERS
>>>>> for
>>>>>      all writable images to allow for OpenGL textures in the lower
>>>>> slots.
>>>>
>>>>
>>>> If pipe_sampler_view  and pipe_image_view are the same, we could
>>>> indeed use
>>>> one structure for both.  While still keeping the separate
>>>> create/bind/destroy functions.
>>>
>>> The big difference is that a sampler view has a first/last layer and
>>> first/last level, while image views are more like surfaces which just
>>> have the one of each. But they also need a byte range for buffer
>>> images.
>>
>> D3D11_TEX2D_ARRAY_UAV allows to specify first/last layer
>> https://msdn.microsoft.com/en-us/library/windows/desktop/ff476242.aspx ,
>> so it sounds that once pipe_image_view is updated to handle D3D11, the
>> difference would reduce to the absence of last_level
>>
>>> Of course we could just ignore that and guarantee that first==last for
>>> images.
>>
>> Yes, it might not be a bad idea.
>>
>
> You could of course argue then isn't it really more like pipe_surface?
> At least in d3d11 clearly they are much closer in concept to rts. The
> actual structures are of course mostly the same in gallium, the
> differences boil down to pipe_surface having (long obsolete)
> width/height parameters and a writable flag, whereas sampler views
> instead have swizzling fields (I don't think they'd have any use for
> this), support multiple levels (again, not needed for shader images /
> uavs), and have a target parameter (in d3d10, rts actually have a target
> parameter too, but it is of no practical consequence, hence there was no
> need for that in gallium - I'm not sure if it would be required for
> shader images / uavs, uavs certainly have such target parameter too but
> I'm not sure it matters).
> But in any case, I'm pretty impartial to what structure is used, as long
> as it is created/destroyed separately.
>
> Roland
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Jul 9, 2015 at 5:05 PM, Marek Olšák <maraeo@gmail.com> wrote:
> I'd like to discuss one more thing that will affect whether image
> slots will be global (shared by all shaders) or not.
>
> Which image unit an image uniform uses is not a compile-time thing,
> but it's specified later using glUniform1i. That means we need a
> per-shader table that maps image uniforms to global image units. One
> possible solution is to add this pipe_context function:
>
> void (*set_shader_image_mapping)(
>    struct pipe_context *, unsigned shader,
>    unsigned start_decl_index, unsigned count,
>    unsigned *decl_to_slot_mapping);
>
> This is only required if the shader image slots are global and not
> per-shader. (if they are per-shader, st/mesa can reorder the slots for
> each shader independently just like it already does for textures and
> UBOs) Shader storage buffer objects suffer from the same issue. Atomic
> counters don't.
>
> Therefore, image slots must be per-shader (like sampler slots) to
> avoid this craziness and keep things simple.

I think that's reasonable. The image slots can be fixed at
compile-time (like sampler slots), and we can just stick the right
images into the right places. But in order to do that, we need the
per-stage mapping, so making it like sampler views seems reasonable.

  -ilia
On Thu, Jul 9, 2015 at 5:30 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Thu, Jul 9, 2015 at 5:05 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> I'd like to discuss one more thing that will affect whether image
>> slots will be global (shared by all shaders) or not.
>>
>> Which image unit an image uniform uses is not a compile-time thing,
>> but it's specified later using glUniform1i. That means we need a
>> per-shader table that maps image uniforms to global image units. One
>> possible solution is to add this pipe_context function:
>>
>> void (*set_shader_image_mapping)(
>>    struct pipe_context *, unsigned shader,
>>    unsigned start_decl_index, unsigned count,
>>    unsigned *decl_to_slot_mapping);
>>
>> This is only required if the shader image slots are global and not
>> per-shader. (if they are per-shader, st/mesa can reorder the slots for
>> each shader independently just like it already does for textures and
>> UBOs) Shader storage buffer objects suffer from the same issue. Atomic
>> counters don't.
>>
>> Therefore, image slots must be per-shader (like sampler slots) to
>> avoid this craziness and keep things simple.
>
> I think that's reasonable. The image slots can be fixed at
> compile-time (like sampler slots), and we can just stick the right
> images into the right places. But in order to do that, we need the
> per-stage mapping, so making it like sampler views seems reasonable.

Oh, except that for textures there's a BIND_TIC/BIND_TSC per shader
stage that lets you point a particular index at an arbitrary
texture/sampler. But the IMAGE things are global from what I can tell
-- there's no extra remapping stage. Ugh, I really don't want to have
to introduce shader variants :( I guess I could just expose images in
fragment shaders.

On Kepler, this should all work out. For texturing, you point it at a
constant buffer that contains the relevant data, I suspect it's
similar for images.
On 09/07/15 22:05, Marek Olšák wrote:
> I'd like to discuss one more thing that will affect whether image
> slots will be global (shared by all shaders) or not.
>
> Which image unit an image uniform uses is not a compile-time thing,
> but it's specified later using glUniform1i. That means we need a
> per-shader table that maps image uniforms to global image units. One
> possible solution is to add this pipe_context function:
>
> void (*set_shader_image_mapping)(
>     struct pipe_context *, unsigned shader,
>     unsigned start_decl_index, unsigned count,
>     unsigned *decl_to_slot_mapping);
>
> This is only required if the shader image slots are global and not
> per-shader. (if they are per-shader, st/mesa can reorder the slots for
> each shader independently just like it already does for textures and
> UBOs) Shader storage buffer objects suffer from the same issue. Atomic
> counters don't.
>
> Therefore, image slots must be per-shader (like sampler slots) to
> avoid this craziness and keep things simple.

Sounds OK to me too.

D3D11's UAV binding points are global, but that can be easily 
accomodated by binding the same UAVs array on all stages.

Jose