Mesa (main): anv: Flip around the way we reason about storage image lowering
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Mon Oct 11 16:53:20 UTC 2021
Module: Mesa
Branch: main
Commit: c0093c466850d51a80f209cc7a0d742ac4fc64bb
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=c0093c466850d51a80f209cc7a0d742ac4fc64bb
Author: Jason Ekstrand <jason at jlekstrand.net>
Date: Wed Oct 6 16:37:03 2021 -0500
anv: Flip around the way we reason about storage image lowering
There are roughly two cases when it comes to storage images. In the
easy case, we have full hardware support and we can just emit a typed
read/write message in the shader and we're done. In the more complex
cases, we may need to fall back to a typed read with a different format
or even to a raw (SSBO) read.
The hardware has always had basically full support for typed writes all
the way back to Ivy Bridge but typed reads have been harder to come by.
Starting with Skylake, we finally have enough that we at least have a
format of the right bit size but not necessarily the right format so we
can use a typed read but may still have to do an int->unorm or similar
cast in the shader.
Previously, in ANV, we treated lowered images as the default and write-
only as a special case that we can optimize. This flips everything
around and treats the cases where we don't need to do any lowering as
the default "vanilla" case and treats the lowered case as special.
Importantly, this means that read-write access to surfaces where the
native format handles typed writes now use the same surface state as
write-only access and the only thing that uses the lowered surface state
is access read-write access with a format that doesn't support typed
reads. This has the added benefit that now, if someone does a read
without specifying a format, we can default to the vanilla surface and
it will work as long as it's a format that supports typed reads.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/13198>
---
src/intel/vulkan/anv_descriptor_set.c | 16 +++---
src/intel/vulkan/anv_image.c | 72 ++++++++++++------------
src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 18 ++++--
src/intel/vulkan/anv_private.h | 24 ++++----
src/intel/vulkan/genX_cmd_buffer.c | 9 +--
5 files changed, 74 insertions(+), 65 deletions(-)
diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_descriptor_set.c
index 578ecfb9b93..401fabcdae3 100644
--- a/src/intel/vulkan/anv_descriptor_set.c
+++ b/src/intel/vulkan/anv_descriptor_set.c
@@ -1360,10 +1360,10 @@ anv_descriptor_set_write_image_view(struct anv_device *device,
assert(!(bind_layout->data & ANV_DESCRIPTOR_IMAGE_PARAM));
assert(image_view->n_planes == 1);
struct anv_storage_image_descriptor desc_data = {
- .read_write = anv_surface_state_to_handle(
+ .vanilla = anv_surface_state_to_handle(
image_view->planes[0].storage_surface_state.state),
- .write_only = anv_surface_state_to_handle(
- image_view->planes[0].writeonly_storage_surface_state.state),
+ .lowered = anv_surface_state_to_handle(
+ image_view->planes[0].lowered_storage_surface_state.state),
};
memcpy(desc_map, &desc_data, sizeof(desc_data));
}
@@ -1372,7 +1372,7 @@ anv_descriptor_set_write_image_view(struct anv_device *device,
/* Storage images can only ever have one plane */
assert(image_view->n_planes == 1);
const struct brw_image_param *image_param =
- &image_view->planes[0].storage_image_param;
+ &image_view->planes[0].lowered_storage_image_param;
anv_descriptor_set_write_image_param(desc_map, image_param);
}
@@ -1437,17 +1437,17 @@ anv_descriptor_set_write_buffer_view(struct anv_device *device,
if (bind_layout->data & ANV_DESCRIPTOR_STORAGE_IMAGE) {
assert(!(bind_layout->data & ANV_DESCRIPTOR_IMAGE_PARAM));
struct anv_storage_image_descriptor desc_data = {
- .read_write = anv_surface_state_to_handle(
+ .vanilla = anv_surface_state_to_handle(
buffer_view->storage_surface_state),
- .write_only = anv_surface_state_to_handle(
- buffer_view->writeonly_storage_surface_state),
+ .lowered = anv_surface_state_to_handle(
+ buffer_view->lowered_storage_surface_state),
};
memcpy(desc_map, &desc_data, sizeof(desc_data));
}
if (bind_layout->data & ANV_DESCRIPTOR_IMAGE_PARAM) {
anv_descriptor_set_write_image_param(desc_map,
- &buffer_view->storage_image_param);
+ &buffer_view->lowered_storage_image_param);
}
}
diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
index bcc2e8de2cd..99050a1ce34 100644
--- a/src/intel/vulkan/anv_image.c
+++ b/src/intel/vulkan/anv_image.c
@@ -2387,7 +2387,7 @@ anv_image_fill_surface_state(struct anv_device *device,
anv_image_address(image, &surface->memory_range);
if (view_usage == ISL_SURF_USAGE_STORAGE_BIT &&
- !(flags & ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY) &&
+ (flags & ANV_IMAGE_VIEW_STATE_STORAGE_LOWERED) &&
!isl_has_matching_typed_storage_image_format(&device->info,
view.format)) {
/* In this case, we are a writeable storage buffer which needs to be
@@ -2407,7 +2407,7 @@ anv_image_fill_surface_state(struct anv_device *device,
state_inout->clear_address = ANV_NULL_ADDRESS;
} else {
if (view_usage == ISL_SURF_USAGE_STORAGE_BIT &&
- !(flags & ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY)) {
+ (flags & ANV_IMAGE_VIEW_STATE_STORAGE_LOWERED)) {
/* Typed surface reads support a very limited subset of the shader
* image formats. Translate it into the closest format the hardware
* supports.
@@ -2628,37 +2628,38 @@ anv_CreateImageView(VkDevice _device,
/* NOTE: This one needs to go last since it may stomp isl_view.format */
if (iview->vk.usage & VK_IMAGE_USAGE_STORAGE_BIT) {
+ iview->planes[vplane].storage_surface_state.state = alloc_surface_state(device);
+ anv_image_fill_surface_state(device, image, 1ULL << iaspect_bit,
+ &iview->planes[vplane].isl,
+ ISL_SURF_USAGE_STORAGE_BIT,
+ ISL_AUX_USAGE_NONE, NULL,
+ 0,
+ &iview->planes[vplane].storage_surface_state,
+ NULL);
+
if (isl_is_storage_image_format(format.isl_format)) {
- iview->planes[vplane].storage_surface_state.state =
+ iview->planes[vplane].lowered_storage_surface_state.state =
alloc_surface_state(device);
anv_image_fill_surface_state(device, image, 1ULL << iaspect_bit,
&iview->planes[vplane].isl,
ISL_SURF_USAGE_STORAGE_BIT,
ISL_AUX_USAGE_NONE, NULL,
- 0,
- &iview->planes[vplane].storage_surface_state,
- &iview->planes[vplane].storage_image_param);
+ ANV_IMAGE_VIEW_STATE_STORAGE_LOWERED,
+ &iview->planes[vplane].lowered_storage_surface_state,
+ &iview->planes[vplane].lowered_storage_image_param);
} else {
/* In this case, we support the format but, because there's no
- * SPIR-V format specifier corresponding to it, we only support
- * NonReadable (writeonly in GLSL) access. Instead of hanging in
- * these invalid cases, we give them a NULL descriptor.
+ * SPIR-V format specifier corresponding to it, we only support it
+ * if the hardware can do it natively. This is possible for some
+ * reads but for most writes. Instead of hanging if someone gets
+ * it wrong, we give them a NULL descriptor.
*/
assert(isl_format_supports_typed_writes(&device->info,
format.isl_format));
iview->planes[vplane].storage_surface_state.state =
device->null_surface_state;
}
-
- iview->planes[vplane].writeonly_storage_surface_state.state = alloc_surface_state(device);
- anv_image_fill_surface_state(device, image, 1ULL << iaspect_bit,
- &iview->planes[vplane].isl,
- ISL_SURF_USAGE_STORAGE_BIT,
- ISL_AUX_USAGE_NONE, NULL,
- ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY,
- &iview->planes[vplane].writeonly_storage_surface_state,
- NULL);
}
}
@@ -2697,9 +2698,9 @@ anv_DestroyImageView(VkDevice _device, VkImageView _iview,
iview->planes[plane].storage_surface_state.state);
}
- if (iview->planes[plane].writeonly_storage_surface_state.state.offset) {
+ if (iview->planes[plane].lowered_storage_surface_state.state.offset) {
anv_state_pool_free(&device->surface_state_pool,
- iview->planes[plane].writeonly_storage_surface_state.state);
+ iview->planes[plane].lowered_storage_surface_state.state);
}
}
@@ -2746,32 +2747,31 @@ anv_CreateBufferView(VkDevice _device,
if (buffer->usage & VK_BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT) {
view->storage_surface_state = alloc_surface_state(device);
- view->writeonly_storage_surface_state = alloc_surface_state(device);
+ view->lowered_storage_surface_state = alloc_surface_state(device);
+
+ anv_fill_buffer_surface_state(device, view->storage_surface_state,
+ view->format, ISL_SURF_USAGE_STORAGE_BIT,
+ view->address, view->range,
+ isl_format_get_layout(view->format)->bpb / 8);
- enum isl_format storage_format =
+ enum isl_format lowered_format =
isl_has_matching_typed_storage_image_format(&device->info,
view->format) ?
isl_lower_storage_image_format(&device->info, view->format) :
ISL_FORMAT_RAW;
- anv_fill_buffer_surface_state(device, view->storage_surface_state,
- storage_format, ISL_SURF_USAGE_STORAGE_BIT,
- view->address, view->range,
- (storage_format == ISL_FORMAT_RAW ? 1 :
- isl_format_get_layout(storage_format)->bpb / 8));
-
- /* Write-only accesses should use the original format. */
- anv_fill_buffer_surface_state(device, view->writeonly_storage_surface_state,
- view->format, ISL_SURF_USAGE_STORAGE_BIT,
+ anv_fill_buffer_surface_state(device, view->lowered_storage_surface_state,
+ lowered_format, ISL_SURF_USAGE_STORAGE_BIT,
view->address, view->range,
- isl_format_get_layout(view->format)->bpb / 8);
+ (lowered_format == ISL_FORMAT_RAW ? 1 :
+ isl_format_get_layout(lowered_format)->bpb / 8));
isl_buffer_fill_image_param(&device->isl_dev,
- &view->storage_image_param,
+ &view->lowered_storage_image_param,
view->format, view->range);
} else {
view->storage_surface_state = (struct anv_state){ 0 };
- view->writeonly_storage_surface_state = (struct anv_state){ 0 };
+ view->lowered_storage_surface_state = (struct anv_state){ 0 };
}
*pView = anv_buffer_view_to_handle(view);
@@ -2797,9 +2797,9 @@ anv_DestroyBufferView(VkDevice _device, VkBufferView bufferView,
anv_state_pool_free(&device->surface_state_pool,
view->storage_surface_state);
- if (view->writeonly_storage_surface_state.alloc_size > 0)
+ if (view->lowered_storage_surface_state.alloc_size > 0)
anv_state_pool_free(&device->surface_state_pool,
- view->writeonly_storage_surface_state);
+ view->lowered_storage_surface_state);
vk_object_free(&device->vk, pAllocator, view);
}
diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
index 0f508490110..d39eae12839 100644
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -1003,6 +1003,13 @@ lower_get_ssbo_size(nir_builder *b, nir_intrinsic_instr *intrin,
return true;
}
+static bool
+image_binding_needs_lowered_surface(nir_variable *var)
+{
+ return !(var->data.access & ACCESS_NON_READABLE) &&
+ var->data.image.format != PIPE_FORMAT_NONE;
+}
+
static bool
lower_image_intrinsic(nir_builder *b, nir_intrinsic_instr *intrin,
struct apply_pipeline_layout_state *state)
@@ -1031,11 +1038,11 @@ lower_image_intrinsic(nir_builder *b, nir_intrinsic_instr *intrin,
nir_ssa_def_rewrite_uses(&intrin->dest.ssa, desc);
} else if (binding_offset > MAX_BINDING_TABLE_SIZE) {
- const bool write_only =
- (var->data.access & ACCESS_NON_READABLE) != 0;
+ const unsigned desc_comp =
+ image_binding_needs_lowered_surface(var) ? 1 : 0;
nir_ssa_def *desc =
build_load_var_deref_descriptor_mem(b, deref, 0, 2, 32, state);
- nir_ssa_def *handle = nir_channel(b, desc, write_only ? 1 : 0);
+ nir_ssa_def *handle = nir_channel(b, desc, desc_comp);
nir_rewrite_image_intrinsic(intrin, handle, true);
} else {
unsigned array_size =
@@ -1609,9 +1616,8 @@ anv_nir_apply_pipeline_layout(const struct anv_physical_device *pdevice,
dim == GLSL_SAMPLER_DIM_SUBPASS_MS)
pipe_binding[i].input_attachment_index = var->data.index + i;
- /* NOTE: This is a uint8_t so we really do need to != 0 here */
- pipe_binding[i].write_only =
- (var->data.access & ACCESS_NON_READABLE) != 0;
+ pipe_binding[i].lowered_storage_surface =
+ image_binding_needs_lowered_surface(var);
}
}
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 159ce4cc1c0..ef804260f68 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1825,8 +1825,8 @@ struct anv_storage_image_descriptor {
* These are expected to already be shifted such that the 20-bit
* SURFACE_STATE table index is in the top 20 bits.
*/
- uint32_t read_write;
- uint32_t write_only;
+ uint32_t vanilla;
+ uint32_t lowered;
};
/** Struct representing a address/range descriptor
@@ -2027,9 +2027,9 @@ struct anv_buffer_view {
struct anv_state surface_state;
struct anv_state storage_surface_state;
- struct anv_state writeonly_storage_surface_state;
+ struct anv_state lowered_storage_surface_state;
- struct brw_image_param storage_image_param;
+ struct brw_image_param lowered_storage_image_param;
};
struct anv_push_descriptor_set {
@@ -2226,8 +2226,8 @@ struct anv_pipeline_binding {
uint8_t dynamic_offset_index;
};
- /** For a storage image, whether it is write-only */
- uint8_t write_only;
+ /** For a storage image, whether it requires a lowered surface */
+ uint8_t lowered_storage_surface;
/** Pad to 64 bits so that there are no holes and we can safely memcmp
* assuming POD zero-initialization.
@@ -4345,18 +4345,20 @@ struct anv_image_view {
/**
* RENDER_SURFACE_STATE when using image as a storage image. Separate
- * states for write-only and readable, using the real format for
- * write-only and the lowered format for readable.
+ * states for vanilla (with the original format) and one which has been
+ * lowered to a format suitable for reading. This may be a raw surface
+ * in extreme cases or simply a surface with a different format where we
+ * expect some conversion to be done in the shader.
*/
struct anv_surface_state storage_surface_state;
- struct anv_surface_state writeonly_storage_surface_state;
+ struct anv_surface_state lowered_storage_surface_state;
- struct brw_image_param storage_image_param;
+ struct brw_image_param lowered_storage_image_param;
} planes[3];
};
enum anv_image_view_state_flags {
- ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY = (1 << 0),
+ ANV_IMAGE_VIEW_STATE_STORAGE_LOWERED = (1 << 0),
ANV_IMAGE_VIEW_STATE_TEXTURE_OPTIMAL = (1 << 1),
};
diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
index 38250983b48..b122628a882 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2762,8 +2762,9 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE: {
if (desc->image_view) {
- struct anv_surface_state sstate = (binding->write_only)
- ? desc->image_view->planes[binding->plane].writeonly_storage_surface_state
+ struct anv_surface_state sstate =
+ binding->lowered_storage_surface
+ ? desc->image_view->planes[binding->plane].lowered_storage_surface_state
: desc->image_view->planes[binding->plane].storage_surface_state;
surface_state = sstate.state;
assert(surface_state.alloc_size);
@@ -2846,8 +2847,8 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
case VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER:
if (desc->buffer_view) {
- surface_state = (binding->write_only)
- ? desc->buffer_view->writeonly_storage_surface_state
+ surface_state = binding->lowered_storage_surface
+ ? desc->buffer_view->lowered_storage_surface_state
: desc->buffer_view->storage_surface_state;
assert(surface_state.alloc_size);
if (need_client_mem_relocs) {
More information about the mesa-commit
mailing list