<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 28, 2017 at 11:35 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Feb 28, 2017 at 08:16:43AM -0800, Jason Ekstrand wrote: > On Tue, Feb 28, 2017 at 5:54 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > On Mon, Feb 27, 2017 at 08:41:56PM -0800, Jason Ekstrand wrote: > > > On Feb 27, 2017 5:21 PM, "Nanley Chery" <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > > > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > > --- > > > src/intel/vulkan/genX_cmd_buffer.c | 57 ++++++++++-------------------- > > > -------- > > > 1 file changed, 15 insertions(+), 42 deletions(-) > > > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c > > > b/src/intel/vulkan/genX_cmd_buffer.c > > > index 5171e6f587..60230bf14b 100644 > > > --- a/src/intel/vulkan/genX_cmd_buffer.c > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c > > > @@ -326,27 +326,6 @@ need_input_attachment_state(const struct > > > anv_render_pass_attachment *att) > > > return vk_format_is_color(att->format); > > > } > > > > > > -static enum isl_aux_usage > > > -layout_to_hiz_usage(VkImageLayout layout, uint8_t samples) > > > -{ > > > - switch (layout) { > > > - case VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL: > > > - return ISL_AUX_USAGE_HIZ; > > > - case VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL: > > > - case VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL: > > > - if (anv_can_sample_with_hiz(GEN_GEN, samples)) > > > - return ISL_AUX_USAGE_HIZ; > > > - /* Fall-through */ > > > - case VK_IMAGE_LAYOUT_GENERAL: > > > - /* This buffer could be used as a source or destination in a > > transfer > > > - * operation. Transfer operations current don't perform > > HiZ-enabled > > > reads > > > - * and writes. > > > - */ > > > - default: > > > - return ISL_AUX_USAGE_NONE; > > > - } > > > -} > > > - > > > /* Transitions a HiZ-enabled depth buffer from one layout to another. > > > Unless > > > * the initial layout is undefined, the HiZ buffer and depth buffer will > > > * represent the same data at the end of this operation. > > > @@ -362,21 +341,15 @@ transition_depth_buffer(struct anv_cmd_buffer > > > *cmd_buffer, > > > if (image->aux_usage != ISL_AUX_USAGE_HIZ || final_layout == > > > initial_layout) > > > return; > > > > > > - const bool hiz_enabled = layout_to_hiz_usage(initial_layout, > > > image->samples) == > > > - ISL_AUX_USAGE_HIZ; > > > - const bool enable_hiz = layout_to_hiz_usage(final_layout, > > > image->samples) == > > > - ISL_AUX_USAGE_HIZ; > > > + const bool hiz_enabled = ISL_AUX_USAGE_HIZ == > > > + anv_layout_to_aux_usage(GEN_GEN, image, image->aspects, > > > + initial_layout, final_layout); > > > + const bool enable_hiz = ISL_AUX_USAGE_HIZ == > > > + anv_layout_to_aux_usage(GEN_GEN, image, image->aspects, > > > + final_layout, final_layout); > > > > > > enum blorp_hiz_op hiz_op; > > > - if (initial_layout == VK_IMAGE_LAYOUT_UNDEFINED) { > > > > > > > > > I'm not sure how I feel about handling this in layout_to_aux_usage. It > > > seems like a conflation of two things. If this is the only reason for > > the > > > future_layout parameter, then it seems like it would be better handled > > > here. In any case, I'll keep reading > > > > > > > Yes, that is the only reason for the future_layout parameter. I can > > remove it if you'd like. > > > > Currently the layout_to_aux_usage function determines the optimal > > aux_usage for all device accesses: sampling, rendering, transferring, > > and resolves. Removing the future_layout parameter would remove its > > responsibility for the last access type. > > > > In my brain, a transition from LAYOUT_UNDEFINED to LAYOUT_* is a transition > from AUX_USAGE_NONE to AUX_USAGE_*. However, we know based on some other > bits of the driver, that we can skip that particular resolve because the > HiZ surface is in a non-hanging state. </div></div>How does this tie into removing the parameter? </blockquote><div> </div><div>I would argue that the obvious thing for a function named "layout_to_aux_usage" to return for UNDEFINED and PREINITIALIZED would be AUX_USAGE_NONE. The fact that resolves want to skip that case doesn't seem to affect the fact that UNDEFINED and PREINITIALIZED things effectively have no aux. It's more of a resolve optimization than a fundamental thing about layouts. </div><div>That's the way it works in my brain. But we have different brains. :-) </div><div>It sounds like you almost want an AUX_USAGE_DONT_CARE for those cases. However, since the only thing that cares is resolves, we can just special-case it there. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > Also, the second parameter makes the function far less obvious and > every other call sight of the function, even though it doesn't care, > has to think about that parmeter. > I'll remove the responsibility of finding the optimal resolving aux_usage from the function in v2. -Nanley <div class="HOEnZb"><div class="h5"> > > > -Nanley > > > > > - /* We've already initialized the aux HiZ buffer at BindImageMemory > > > time, > > > - * so there's no need to perform a HIZ resolve or clear to avoid > > GPU > > > hangs. > > > - * This initial layout indicates that the user doesn't care about > > > the data > > > - * that's currently in the buffer, so resolves are not necessary > > > except > > > - * for the special case noted below. > > > - */ > > > - hiz_op = BLORP_HIZ_OP_NONE; > > > - } else if (hiz_enabled && !enable_hiz) { > > > + if (hiz_enabled && !enable_hiz) { > > > hiz_op = BLORP_HIZ_OP_DEPTH_RESOLVE; > > > } else if (!hiz_enabled && enable_hiz) { > > > hiz_op = BLORP_HIZ_OP_HIZ_RESOLVE; > > > @@ -556,12 +529,11 @@ genX(cmd_buffer_setup_attachments)(struct > > > anv_cmd_buffer *cmd_buffer, > > > state->attachments[i].aux_usage, > > > state->attachments[i].color_ > > rt_state); > > > } else { > > > - if (iview->image->aux_usage == ISL_AUX_USAGE_HIZ) { > > > - state->attachments[i].aux_usage = > > > - layout_to_hiz_usage(att->initial_layout, > > > iview->image->samples); > > > - } else { > > > - state->attachments[i].aux_usage = ISL_AUX_USAGE_NONE; > > > - } > > > + /* This field will be initialized after the first subpass > > > + * transition. > > > + */ > > > + state->attachments[i].aux_usage = ISL_AUX_USAGE_NONE; > > > + > > > state->attachments[i].input_aux_usage = ISL_AUX_USAGE_NONE; > > > } > > > > > > @@ -2399,8 +2371,9 @@ genX(cmd_buffer_set_subpass)(struct anv_cmd_buffer > > > *cmd_buffer, > > > cmd_buffer->state.attachments[ds].current_layout = > > > cmd_buffer->state.subpass->depth_stencil_layout; > > > cmd_buffer->state.attachments[ds].aux_usage = > > > - layout_to_hiz_usage(cmd_buffer->state.subpass->depth_stenci > > > l_layout, > > > - iview->image->samples); > > > + anv_layout_to_aux_usage(GEN_GEN, iview->image, > > iview->aspect_mask, > > > + cmd_buffer->state.subpass->depth_stencil_layout, > > > + cmd_buffer->state.subpass->depth_stencil_layout); > > > } > > > > > > cmd_buffer_emit_depth_stencil(cmd_buffer); > > > -- > > > 2.11.1 > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>