On Wed, Jan 24, 2018 at 02:32:02PM -0800, Jason Ekstrand wrote:
> On Wed, Jan 24, 2018 at 1:59 PM, Nanley Chery <nanleych...@gmail.com> wrote:
>
> > On Mon, Jan 22, 2018 at 06:39:52PM -0800, Jason Ekstrand wrote:
> > > On Mon, Jan 22, 2018 at 5:50 PM, Jason Ekstrand <ja...@jlekstrand.net>
> > > wrote:
> > >
> > > > On Mon, Jan 22, 2018 at 11:31 AM, Nanley Chery <nanleych...@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.
> > > >
> > > >
> >
> > Sounds good.
> >
> > > >> > 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_ATTACHM
> > > >> ENT_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.
> > > >
> > >
> > > Gah! As I was working on this, I realized that the reason I hadn't
> > broken
> > > it out into a separate function is that we need some of the intermediate
> > > results for actually building the predicate and doing the resolve. What
> > I
> > > propose to do is to move the assert up above the "if (initial_aux_usage
> > ==
> > > ISL_AUX_USAGE_NONE) return;" and change the comment to the following:
> > >
> > > /* The current code assumes that there is no mixing of CCS_E and
> > CCS_D.
> > > * We can handle transitions between CCS_D/E to and from NONE. What
> > we
> > > * don't yet handle is switching between CCS_E and CCS_D within a
> > given
> > > * image. Doing so in a performant way requires more detailed aux
> > state
> > > * tracking such as what is done in i965. For now, just assume that
> > we
> > > * only have one type of compression.
> > > */
> > >
> > >
> >
> > This change looks good. Though, if I'm not mistaken, we'll have enough aux
> > tracking to do the transition in a performant manner by
> > the end of the series right?
> >
>
> Maybe? I haven't thought through all the corners yet. It's entirely
> possible that this code is sufficient but I'm not sure. At the moment,
> it's not something we do because we pick between CCS_E and CCS_D at image
> creation time. This assert stands as a note that, if we ever relax that,
> we someone needs to go have a good think.
>
Alright. With your updates applied, this patch is
Reviewed-by: Nanley Chery <nanley.g.ch...@intel.com>
> --Jason
>
>
> > -Nanley
> >
> > > > 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
