On Fri, Jan 19, 2018 at 03:47:26PM -0800, Jason Ekstrand wrote:
> This moves it to being based on layout_to_aux_usage instead of being
> hard-coded based on bits of a priori knowledge of how transitions
> interact with layouts. This conceptually simplifies things because
> we're now using layout_to_aux_usage and layout_supports_fast_clear to
> make resolve decisions so changes to those functions will do what one
> expects.
>
> This fixes a potential bug with window system integration on gen9+ where
^
This patch still doesn't fix the bug.
> we wouldn't do a resolve when transitioning to the PRESENT_SRC layout
> because we just assume that everything that handles CCS_E can handle it
> all the time. When handing a CCS_E image off to the window system, we
> may need to do a full resolve if the window system does not support the
> CCS_E modifier. The only reason why this hasn't been a problem yet is
> because we don't support modifiers in Vulkan WSI and so we always get X
> tiling which implies no CCS on gen9+.
>
> v2 (Jason Ekstrand):
> - Make a few more things const
> - Use the anv_fast_clear_support enum
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> ---
> src/intel/vulkan/genX_cmd_buffer.c | 56
> ++++++++++++++++++++++++++++++--------
> 1 file changed, 44 insertions(+), 12 deletions(-)
>
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 6a6d8b2..fd27463 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -593,6 +593,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
> VkImageLayout initial_layout,
> VkImageLayout final_layout)
> {
> + const struct gen_device_info *devinfo = &cmd_buffer->device->info;
> /* Validate the inputs. */
> assert(cmd_buffer);
> assert(image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV);
> @@ -733,17 +734,51 @@ transition_color_buffer(struct anv_cmd_buffer
> *cmd_buffer,
> VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL,
> final_layout);
> }
> - } else if (initial_layout != VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL) {
> - /* Resolves are only necessary if the subresource may contain blocks
> - * fast-cleared to values unsupported in other layouts. This only
> occurs
> - * if the initial layout is COLOR_ATTACHMENT_OPTIMAL.
> - */
> - return;
> - } else if (image->samples > 1) {
> - /* MCS buffers don't need resolving. */
> return;
> }
>
> + /* If initial aux usage is NONE, there is nothing to resolve */
> + const enum isl_aux_usage initial_aux_usage =
> + anv_layout_to_aux_usage(devinfo, image, aspect, initial_layout);
> + if (initial_aux_usage == ISL_AUX_USAGE_NONE)
> + return;
> +
> + enum isl_aux_op resolve_op = ISL_AUX_OP_NONE;
> +
> + /* If the initial layout supports more fast clear than the final layout
> + * then we need at least a partial resolve.
> + */
> + const enum anv_fast_clear_type initial_fast_clear =
> + anv_layout_to_fast_clear_type(devinfo, image, aspect, initial_layout);
> + const enum anv_fast_clear_type final_fast_clear =
> + anv_layout_to_fast_clear_type(devinfo, image, aspect, final_layout);
> + if (final_fast_clear < initial_fast_clear)
> + resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
> +
> + const enum isl_aux_usage final_aux_usage =
> + anv_layout_to_aux_usage(devinfo, image, aspect, final_layout);
> + if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&
> + final_aux_usage != ISL_AUX_USAGE_CCS_E)
> + resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> +
> + /* CCS_D only supports full resolves and BLORP will assert on us if we try
> + * to do a partial resolve on a CCS_D surface.
> + */
> + if (resolve_op == ISL_AUX_OP_PARTIAL_RESOLVE &&
> + initial_aux_usage == ISL_AUX_USAGE_CCS_D)
> + resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> +
> + if (resolve_op == ISL_AUX_OP_NONE)
> + return;
> +
> + /* Even though the above code can theoretically handle multiple resolve
> + * types such as CCS_D -> CCS_E, the predication code below can't. We
> only
> + * really handle a couple of cases.
> + */
> + assert(initial_aux_usage == ISL_AUX_USAGE_NONE ||
> + final_aux_usage == ISL_AUX_USAGE_NONE ||
> + initial_aux_usage == final_aux_usage);
> +
I'm finding this assertion and comment confusing. The comment says that
the predication code below can't handle CCS_D -> CCS_E (which requires a
no-op resolve), but the assertion below it allows initial_aux_usage to
be NONE (which would lead to a no-op resolve), and initial_aux_usage ==
final_aux_usage which (may lead to a no-op resolve).
As far as I can tell, the only problematic case this assertion would catch
is a CCS_E -> CCS_D transition. This transition requires a FULL_RESOLVE. If
the CCS_E texture was fast-cleared to transparent black then the
needs_resolve predicate would be set false. In this case a resolve would
not occur when it should. Unfortunately, this assertion does allow the
case of CCS_E -> NONE which has the same problem as CCS_E -> CCS_D.
Perhaps we should update the comment to note the difficulty in
transitioning from CCS_E and assert:
if (initial_aux_usage == ISL_AUX_USAGE_CCS_E)
assert(final_aux_usage == ISL_AUX_USAGE_CCS_E);
-Nanley
> /* Perform a resolve to synchronize data between the main and aux buffer.
> * Before we begin, we must satisfy the cache flushing requirement
> specified
> * in the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)":
> @@ -774,10 +809,7 @@ transition_color_buffer(struct anv_cmd_buffer
> *cmd_buffer,
> genX(load_needs_resolve_predicate)(cmd_buffer, image, aspect, level);
>
> anv_image_ccs_op(cmd_buffer, image, aspect, level,
> - base_layer, layer_count,
> - image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E
> ?
> - ISL_AUX_OP_PARTIAL_RESOLVE : ISL_AUX_OP_FULL_RESOLVE,
> - true);
> + base_layer, layer_count, resolve_op, true);
>
> genX(set_image_needs_resolve)(cmd_buffer, image, aspect, level, false);
> }
> --
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
