<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 22, 2018 at 5:50 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-">On Mon, Jan 22, 2018 at 11:31 AM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-m_-2071944783323379741gmail-">On Fri, Jan 19, 2018 at 03:47:26PM -0800, Jason Ekstrand wrote:<br>
</span><span class="gmail-m_-2071944783323379741gmail-">> This moves it to being based on layout_to_aux_usage instead of being<br>
> hard-coded based on bits of a priori knowledge of how transitions<br>
> interact with layouts. This conceptually simplifies things because<br>
> we're now using layout_to_aux_usage and layout_supports_fast_clear to<br>
> make resolve decisions so changes to those functions will do what one<br>
> expects.<br>
><br>
> This fixes a potential bug with window system integration on gen9+ where<br>
</span> ^<br>
This patch still doesn't fix the bug.<br><div><div class="gmail-m_-2071944783323379741gmail-h5"></div></div></blockquote><div><br></div></span><div>Yup. I've changed this paragraph to:</div><div><br></div><div> There is a potential bug with window system integration on gen9+ where<span class="gmail-"><br> we wouldn't do a resolve when transitioning to the PRESENT_SRC layout<br> because we just assume that everything that handles CCS_E can handle it<br> all the time. When handing a CCS_E image off to the window system, we<br> may need to do a full resolve if the window system does not support the<br> CCS_E modifier. The only reason why this hasn't been a problem yet is<br> because we don't support modifiers in Vulkan WSI and so we always get X<br></span> tiling which implies no CCS on gen9+. This patch doesn't actually fix<br> that bug yet but it takes us the first step in that direction by making<br> us actually pick the correct resolve op. In order to handle all of the<br> cases, we need more detailed aux tracking.<br></div><div><div class="gmail-h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-m_-2071944783323379741gmail-h5">
> we wouldn't do a resolve when transitioning to the PRESENT_SRC layout<br>
> because we just assume that everything that handles CCS_E can handle it<br>
> all the time. When handing a CCS_E image off to the window system, we<br>
> may need to do a full resolve if the window system does not support the<br>
> CCS_E modifier. The only reason why this hasn't been a problem yet is<br>
> because we don't support modifiers in Vulkan WSI and so we always get X<br>
> tiling which implies no CCS on gen9+.<br>
><br>
> v2 (Jason Ekstrand):<br>
> - Make a few more things const<br>
> - Use the anv_fast_clear_support enum<br>
><br>
> Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br>
> ---<br>
> src/intel/vulkan/genX_cmd_buff<wbr>er.c | 56 ++++++++++++++++++++++++++++++<wbr>--------<br>
> 1 file changed, 44 insertions(+), 12 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/genX_cmd_bu<wbr>ffer.c b/src/intel/vulkan/genX_cmd_bu<wbr>ffer.c<br>
> index 6a6d8b2..fd27463 100644<br>
> --- a/src/intel/vulkan/genX_cmd_bu<wbr>ffer.c<br>
> +++ b/src/intel/vulkan/genX_cmd_bu<wbr>ffer.c<br>
> @@ -593,6 +593,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,<br>
> VkImageLayout initial_layout,<br>
> VkImageLayout final_layout)<br>
> {<br>
> + const struct gen_device_info *devinfo = &cmd_buffer->device->info;<br>
> /* Validate the inputs. */<br>
> assert(cmd_buffer);<br>
> assert(image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_<wbr>ANV);<br>
> @@ -733,17 +734,51 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,<br>
> VK_IMAGE_LAYOUT_COLOR_ATTACHM<wbr>ENT_OPTIMAL,<br>
> final_layout);<br>
> }<br>
> - } else if (initial_layout != VK_IMAGE_LAYOUT_COLOR_ATTACHME<wbr>NT_OPTIMAL) {<br>
> - /* Resolves are only necessary if the subresource may contain blocks<br>
> - * fast-cleared to values unsupported in other layouts. This only occurs<br>
> - * if the initial layout is COLOR_ATTACHMENT_OPTIMAL.<br>
> - */<br>
> - return;<br>
> - } else if (image->samples > 1) {<br>
> - /* MCS buffers don't need resolving. */<br>
> return;<br>
> }<br>
><br>
> + /* If initial aux usage is NONE, there is nothing to resolve */<br>
> + const enum isl_aux_usage initial_aux_usage =<br>
> + anv_layout_to_aux_usage(devinf<wbr>o, image, aspect, initial_layout);<br>
> + if (initial_aux_usage == ISL_AUX_USAGE_NONE)<br>
> + return;<br>
> +<br>
> + enum isl_aux_op resolve_op = ISL_AUX_OP_NONE;<br>
> +<br>
> + /* If the initial layout supports more fast clear than the final layout<br>
> + * then we need at least a partial resolve.<br>
> + */<br>
> + const enum anv_fast_clear_type initial_fast_clear =<br>
> + anv_layout_to_fast_clear_type(<wbr>devinfo, image, aspect, initial_layout);<br>
> + const enum anv_fast_clear_type final_fast_clear =<br>
> + anv_layout_to_fast_clear_type(<wbr>devinfo, image, aspect, final_layout);<br>
> + if (final_fast_clear < initial_fast_clear)<br>
> + resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;<br>
> +<br>
> + const enum isl_aux_usage final_aux_usage =<br>
> + anv_layout_to_aux_usage(devinf<wbr>o, image, aspect, final_layout);<br>
> + if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&<br>
> + final_aux_usage != ISL_AUX_USAGE_CCS_E)<br>
> + resolve_op = ISL_AUX_OP_FULL_RESOLVE;<br>
> +<br>
> + /* CCS_D only supports full resolves and BLORP will assert on us if we try<br>
> + * to do a partial resolve on a CCS_D surface.<br>
> + */<br>
> + if (resolve_op == ISL_AUX_OP_PARTIAL_RESOLVE &&<br>
> + initial_aux_usage == ISL_AUX_USAGE_CCS_D)<br>
> + resolve_op = ISL_AUX_OP_FULL_RESOLVE;<br>
> +<br>
> + if (resolve_op == ISL_AUX_OP_NONE)<br>
> + return;<br>
> +<br>
> + /* Even though the above code can theoretically handle multiple resolve<br>
> + * types such as CCS_D -> CCS_E, the predication code below can't. We only<br>
> + * really handle a couple of cases.<br>
> + */<br>
> + assert(initial_aux_usage == ISL_AUX_USAGE_NONE ||<br>
> + final_aux_usage == ISL_AUX_USAGE_NONE ||<br>
> + initial_aux_usage == final_aux_usage);<br>
> +<br>
<br>
</div></div>I'm finding this assertion and comment confusing.</blockquote><div><br></div></div></div><div>You and Topi both!<br></div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The comment says that<br>
the predication code below can't handle CCS_D -> CCS_E (which requires a<br>
no-op resolve), but the assertion below it allows initial_aux_usage to<br>
be NONE (which would lead to a no-op resolve), and initial_aux_usage ==<br>
final_aux_usage which (may lead to a no-op resolve).<br>
<br>
As far as I can tell, the only problematic case this assertion would catch<br>
is a CCS_E -> CCS_D transition. This transition requires a FULL_RESOLVE. If<br>
the CCS_E texture was fast-cleared to transparent black then the<br>
needs_resolve predicate would be set false. In this case a resolve would<br>
not occur when it should. Unfortunately, this assertion does allow the<br>
case of CCS_E -> NONE which has the same problem as CCS_E -> CCS_D.<br></blockquote><div><br></div></span><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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?</div><div><br></div><div>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.<br></div></div></div></div></blockquote><div><br></div><div>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:</div><div><br></div><div> /* The current code assumes that there is no mixing of CCS_E and CCS_D.<br> * We can handle transitions between CCS_D/E to and from NONE. What we<br> * don't yet handle is switching between CCS_E and CCS_D within a given<br> * image. Doing so in a performant way requires more detailed aux state<br> * tracking such as what is done in i965. For now, just assume that we<br> * only have one type of compression.<br> */<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Perhaps we should update the comment to note the difficulty in<br>
transitioning from CCS_E and assert:<br>
<br>
if (initial_aux_usage == ISL_AUX_USAGE_CCS_E)<br>
assert(final_aux_usage == ISL_AUX_USAGE_CCS_E);<br>
<br>
-Nanley<br>
<span class="gmail-m_-2071944783323379741gmail-im gmail-m_-2071944783323379741gmail-HOEnZb"><br>
> /* Perform a resolve to synchronize data between the main and aux buffer.<br>
> * Before we begin, we must satisfy the cache flushing requirement specified<br>
> * in the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)":<br>
> @@ -774,10 +809,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,<br>
> genX(load_needs_resolve_predic<wbr>ate)(cmd_buffer, image, aspect, level);<br>
><br>
> anv_image_ccs_op(cmd_buffer, image, aspect, level,<br>
> - base_layer, layer_count,<br>
> - image->planes[plane].aux_<wbr>usage == ISL_AUX_USAGE_CCS_E ?<br>
> - ISL_AUX_OP_PARTIAL_RESOLVE : ISL_AUX_OP_FULL_RESOLVE,<br>
> - true);<br>
> + base_layer, layer_count, resolve_op, true);<br>
><br>
> genX(set_image_needs_resolve)(<wbr>cmd_buffer, image, aspect, level, false);<br>
> }<br>
> --<br>
> 2.5.0.400.gff86faf<br>
><br>
</span><div class="gmail-m_-2071944783323379741gmail-HOEnZb"><div class="gmail-m_-2071944783323379741gmail-h5">> ______________________________<wbr>_________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>