[Mesa-dev] [PATCH 03/18] anv/cmd_buffer: Replace layout_to_hiz_usage()
Nanley Chery
nanleychery at gmail.com
Tue Feb 28 20:47:40 UTC 2017
On Tue, Feb 28, 2017 at 11:49:18AM -0800, Jason Ekstrand wrote:
> On Tue, Feb 28, 2017 at 11:35 AM, Nanley Chery <nanleychery at gmail.com>
> wrote:
>
> > On Tue, Feb 28, 2017 at 08:16:43AM -0800, Jason Ekstrand wrote:
> > > On Tue, Feb 28, 2017 at 5:54 AM, Nanley Chery <nanleychery at gmail.com>
> > wrote:
> > >
> > > > On Mon, Feb 27, 2017 at 08:41:56PM -0800, Jason Ekstrand wrote:
> > > > > On Feb 27, 2017 5:21 PM, "Nanley Chery" <nanleychery at gmail.com>
> > wrote:
> > > > >
> > > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > > ---
> > > > > 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.
> >
> > How does this tie into removing the parameter?
> >
>
> 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.
>
> That's the way it works in my brain. But we have different brains. :-)
>
> 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.
>
I see what you mean. Yes, we'll special case it here. As written, the
function determines the optimal aux_usage for accessing an image of a
given layout. Since the UNDEFINED and PREINITIALIZED layouts are not
device accessible, they will trigger an assert. I think this makes sense
and gives us extra guarantees against unintentionally invalid inputs.
-Nanley
>
> > > 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
> >
> > >
> > > > -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
> > > > > mesa-dev at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >
More information about the mesa-dev
mailing list