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

[Roland Scheidegger]

Am 05.07.2015 um 15:47 schrieb 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.

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