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.
>
>
>> > 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.
*/
> 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
