[Mesa-dev] [RFC] gallium: add interface for writable shader images
Marek Olšák
maraeo at gmail.com
Mon Jul 6 03:47:31 PDT 2015
On Sun, Jul 5, 2015 at 3:47 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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.
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
More information about the mesa-dev
mailing list