[Mesa-dev] [PATCH 3/5] radv: Don't handle DCC in compute resolve.

[Bas Nieuwenhuizen]

If the destination has DCC, we will use the FS resolve.
---
src/amd/vulkan/radv_meta_resolve_cs.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/amd/vulkan/radv_meta_resolve_cs.c b/src/amd/vulkan/radv_meta_resolve_cs.c
index 5b6cea6c103..7c569aa9202 100644
--- a/src/amd/vulkan/radv_meta_resolve_cs.c
+++ b/src/amd/vulkan/radv_meta_resolve_cs.c
@@ -506,10 +506,7 @@ radv_cmd_buffer_resolve_subpass_cs(struct radv_cmd_buffer *cmd_buffer)
struct radv_image *dst_img = cmd_buffer->state.framebuffer->attachments[dest_att.attachment].attachment->image;
struct radv_image_view *src_iview = cmd_buffer->state.framebuffer->attachments[src_att.attachment].attachment;

- if (dst_img->surface.dcc_size) {
- radv_initialize_dcc(cmd_buffer, dst_img, 0xffffffff);
- cmd_buffer->state.attachments[dest_att.attachment].current_layout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL;
- }
+ assert(!dst_img->surface.dcc_size);

VkImageSubresourceRange range;
range.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT;

[Bas Nieuwenhuizen]

The position start at (dst.x, dst.y), so if we want the source to
start at (src.x, src.y), we have to offset by (src.x-dst.x,src.y-dst.y).

Haven't tested that this fixed anything yet, but found by inspection.
Fixes: 69136f4e633 "radv/meta: add resolve pass using fragment/vertex shaders"
---
src/amd/vulkan/radv_meta_resolve_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/amd/vulkan/radv_meta_resolve_fs.c b/src/amd/vulkan/radv_meta_resolve_fs.c
index 756309efd2e..53a3d68284a 100644
--- a/src/amd/vulkan/radv_meta_resolve_fs.c
+++ b/src/amd/vulkan/radv_meta_resolve_fs.c
@@ -407,8 +407,8 @@ emit_resolve(struct radv_cmd_buffer *cmd_buffer,
cmd_buffer->state.flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_CB;

unsigned push_constants[2] = {
- src_offset->x,
- src_offset->y,
+ src_offset->x - dst_offset->x,
+ src_offset->y - dst_offset->y,
};
radv_CmdPushConstants(radv_cmd_buffer_to_handle(cmd_buffer),
device->meta_state.resolve_fragment.p_layout,

[Dieter Nützel]

Post by Bas Nieuwenhuizen
The position start at (dst.x, dst.y), so if we want the source to
start at (src.x, src.y), we have to offset by
(src.x-dst.x,src.y-dst.y).
Haven't tested that this fixed anything yet, but found by inspection.
Fixes: 69136f4e633 "radv/meta: add resolve pass using fragment/vertex shaders"
---
src/amd/vulkan/radv_meta_resolve_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/amd/vulkan/radv_meta_resolve_fs.c
b/src/amd/vulkan/radv_meta_resolve_fs.c
index 756309efd2e..53a3d68284a 100644
--- a/src/amd/vulkan/radv_meta_resolve_fs.c
+++ b/src/amd/vulkan/radv_meta_resolve_fs.c
@@ -407,8 +407,8 @@ emit_resolve(struct radv_cmd_buffer *cmd_buffer,
cmd_buffer->state.flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_CB;
unsigned push_constants[2] = {
- src_offset->x,
- src_offset->y,
+ src_offset->x - dst_offset->x,
+ src_offset->y - dst_offset->y,

Shouldn't that look like this:

+ src_offset->x - dest_offset->x,
+ src_offset->y - dest_offset->y,

Even compilation error is solved.

With this fixed,

series is

Tested-by: Dieter Nützel <***@nuetzel-hh.de>

Dieter

Post by Bas Nieuwenhuizen
};
radv_CmdPushConstants(radv_cmd_buffer_to_handle(cmd_buffer),
device->meta_state.resolve_fragment.p_layout,

[Bas Nieuwenhuizen]

Post by Dieter Nützel

Post by Bas Nieuwenhuizen
The position start at (dst.x, dst.y), so if we want the source to
start at (src.x, src.y), we have to offset by (src.x-dst.x,src.y-dst.y).
Haven't tested that this fixed anything yet, but found by inspection.
Fixes: 69136f4e633 "radv/meta: add resolve pass using fragment/vertex shaders"
---
src/amd/vulkan/radv_meta_resolve_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/amd/vulkan/radv_meta_resolve_fs.c
b/src/amd/vulkan/radv_meta_resolve_fs.c
index 756309efd2e..53a3d68284a 100644
--- a/src/amd/vulkan/radv_meta_resolve_fs.c
+++ b/src/amd/vulkan/radv_meta_resolve_fs.c
@@ -407,8 +407,8 @@ emit_resolve(struct radv_cmd_buffer *cmd_buffer,
cmd_buffer->state.flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_CB;
unsigned push_constants[2] = {
- src_offset->x,
- src_offset->y,
+ src_offset->x - dst_offset->x,
+ src_offset->y - dst_offset->y,

+ src_offset->x - dest_offset->x,
+ src_offset->y - dest_offset->y,
Even compilation error is solved.

Woops, yes indeed.

Post by Dieter Nützel
With this fixed,
series is

Thanks.

Post by Dieter Nützel
Dieter

Post by Bas Nieuwenhuizen
};
radv_CmdPushConstants(radv_cmd_buffer_to_handle(cmd_buffer),
device->meta_state.resolve_fragment.p_layout,

[Bas Nieuwenhuizen]

HW resolve does not support it either.
---
src/amd/vulkan/radv_meta_resolve.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/amd/vulkan/radv_meta_resolve.c b/src/amd/vulkan/radv_meta_resolve.c
index e73a950ab7c..26489b7834f 100644
--- a/src/amd/vulkan/radv_meta_resolve.c
+++ b/src/amd/vulkan/radv_meta_resolve.c
@@ -318,11 +318,10 @@ static void radv_pick_resolve_method_images(struct radv_image *src_image,
enum radv_resolve_method *method)

{
- if (dest_image->surface.micro_tile_mode != src_image->surface.micro_tile_mode) {
- if (dest_image->surface.num_dcc_levels > 0)
- *method = RESOLVE_FRAGMENT;
- else
- *method = RESOLVE_COMPUTE;
+ if (dest_image->surface.num_dcc_levels > 0) {
+ *method = RESOLVE_FRAGMENT;
+ } else if (dest_image->surface.micro_tile_mode != src_image->surface.micro_tile_mode) {
+ *method = RESOLVE_COMPUTE;
}
}

[Bas Nieuwenhuizen]

Fixes: f4e499ec791 "radv: add initial non-conformant radv vulkan driver"
---
src/amd/vulkan/radv_meta_resolve_cs.c | 8 ++++++++
src/amd/vulkan/radv_meta_resolve_fs.c | 10 ++++++++++
2 files changed, 18 insertions(+)

diff --git a/src/amd/vulkan/radv_meta_resolve_cs.c b/src/amd/vulkan/radv_meta_resolve_cs.c
index 3833faa1247..5b6cea6c103 100644
--- a/src/amd/vulkan/radv_meta_resolve_cs.c
+++ b/src/amd/vulkan/radv_meta_resolve_cs.c
@@ -487,6 +487,14 @@ radv_cmd_buffer_resolve_subpass_cs(struct radv_cmd_buffer *cmd_buffer)
if (!subpass->has_resolve)
return;

+ /* Resolves happen before the end-of-subpass barriers get executed,
+ * so we have to make the attachment shader-readable */
+ cmd_buffer->state.flush_bits |= RADV_CMD_FLAG_PS_PARTIAL_FLUSH |
+ RADV_CMD_FLAG_FLUSH_AND_INV_CB |
+ RADV_CMD_FLAG_FLUSH_AND_INV_CB_META |
+ RADV_CMD_FLAG_INV_GLOBAL_L2 |
+ RADV_CMD_FLAG_INV_VMEM_L1;
+
for (uint32_t i = 0; i < subpass->color_count; ++i) {
VkAttachmentReference src_att = subpass->color_attachments[i];
VkAttachmentReference dest_att = subpass->resolve_attachments[i];
diff --git a/src/amd/vulkan/radv_meta_resolve_fs.c b/src/amd/vulkan/radv_meta_resolve_fs.c
index c25aea73670..756309efd2e 100644
--- a/src/amd/vulkan/radv_meta_resolve_fs.c
+++ b/src/amd/vulkan/radv_meta_resolve_fs.c
@@ -604,6 +604,16 @@ radv_cmd_buffer_resolve_subpass_fs(struct radv_cmd_buffer *cmd_buffer)
RADV_META_SAVE_CONSTANTS |
RADV_META_SAVE_DESCRIPTORS);

+ /* Resolves happen before the end-of-subpass barriers get executed,
+ * so we have to make the attachment shader-readable */
+ cmd_buffer->state.flush_bits |= RADV_CMD_FLAG_PS_PARTIAL_FLUSH |
+ RADV_CMD_FLAG_FLUSH_AND_INV_CB |
+ RADV_CMD_FLAG_FLUSH_AND_INV_CB_META |
+ RADV_CMD_FLAG_FLUSH_AND_INV_DB |
+ RADV_CMD_FLAG_FLUSH_AND_INV_DB_META |
+ RADV_CMD_FLAG_INV_GLOBAL_L2 |
+ RADV_CMD_FLAG_INV_VMEM_L1;
+
for (uint32_t i = 0; i < subpass->color_count; ++i) {
VkAttachmentReference src_att = subpass->color_attachments[i];
VkAttachmentReference dest_att = subpass->resolve_attachments[i];

[Dave Airlie]

the samples_identical instruction returns 0 if they are differet, so
we have to do the extra work if the result is 0, not if it is != 0.

I think I lost the logic trail on this code a few times.

Patches look good.

Reviewed-by: Dave Airlie <***@redhat.com>

For the series.

Dave.