Discussion:
[PATCH 3/5] radv: Don't handle DCC in compute resolve.
(too old to reply)
Bas Nieuwenhuizen
2017-12-27 00:20:36 UTC
Permalink
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;
--
2.15.1
Bas Nieuwenhuizen
2017-12-27 00:20:37 UTC
Permalink
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,
--
2.15.1
Dieter Nützel
2017-12-27 04:25:16 UTC
Permalink
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
2017-12-27 11:19:47 UTC
Permalink
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
2017-12-27 00:20:38 UTC
Permalink
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;
}
}
--
2.15.1
Bas Nieuwenhuizen
2017-12-27 00:20:35 UTC
Permalink
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];
--
2.15.1
Dave Airlie
2017-12-27 01:08:09 UTC
Permalink
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.
Loading...