[Mesa-dev] [PATCH v2 09/24] anv/cmd_buffer: Generalize transition_color_buffer
Jason Ekstrand
jason at jlekstrand.net
Tue Jan 23 01:50:59 UTC 2018
On Mon, Jan 22, 2018 at 11:31 AM, Nanley Chery <nanleychery at gmail.com>
wrote:
> 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.
>
Yup. I've changed this paragraph to:
There is a potential bug with window system integration on gen9+ where
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+. This patch doesn't actually fix
that bug yet but it takes us the first step in that direction by making
us actually pick the correct resolve op. In order to handle all of the
cases, we need more detailed aux tracking.
> > 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.pohjolainen at 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.
You and Topi both!
> 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.
>
Ok, let me make things a bit more clear. After reading what you wrote and
what Topi wrote and the code, my memory is jogged as to exactly why I made
the assert the way I did.
The if condition above this which selects partial resolves makes the
assumption that we don't ever mix CCS_E and CCS_D. For a given image, it
can only have one of two aux_usages: NONE and one of CCS_E or CCS_D. If we
want to handle mixing CCS_E and CCS_D, we may need more complex logic like
in i965.
It's entirely possible that the above condition actually does work in all
the cases where CCS_E and CCS_D are mixed but I haven't thought about it
long enough to determine if that is the case. What I really wanted to do
was to assert that we don't have CCS_E/D mixing. Does that make more sense?
Also, I think I said I would break this out into a helper function to make
it make more sense. I'll do that, make the assert make more sense, and
send out a v3.
> 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180122/8323bee0/attachment.html>
More information about the mesa-dev
mailing list