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

[Ilia Mirkin]

On Sun, Jul 5, 2015 at 9:25 AM, Marek Olšák <maraeo at gmail.com> wrote:
> From: Marek Olšák <marek.olsak at 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