On Tue, Feb 13, 2018 at 4:35 PM, Nanley Chery <nanleych...@gmail.com> wrote:
> On Mon, Feb 05, 2018 at 02:35:00PM -0800, Jason Ekstrand wrote:
> > This is quite a bit cleaner because we now sync the clear values at the
> > same time as we do the fast clear. For loading the clear values into
> > the surface state, we now do it once when we handle the LOAD_OP_LOAD
> > instead of every subpass.
> > ---
> > src/intel/vulkan/genX_cmd_buffer.c | 148 ++++++++++++------------------
> -------
> > 1 file changed, 48 insertions(+), 100 deletions(-)
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> > index f92e86f..4eee85a 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -3392,97 +3392,6 @@ cmd_buffer_subpass_transition_layouts(struct
> anv_cmd_buffer * const cmd_buffer,
> > }
> > }
> >
> > -/* Update the clear value dword(s) in surface state objects or the fast
> clear
> > - * state buffer entry for the color attachments used in this subpass.
> > - */
> > -static void
> > -cmd_buffer_subpass_sync_fast_clear_values(struct anv_cmd_buffer
> *cmd_buffer)
> > -{
> > - assert(cmd_buffer && cmd_buffer->state.subpass);
> > -
> > - const struct anv_cmd_state *state = &cmd_buffer->state;
> > -
> > - /* Iterate through every color attachment used in this subpass. */
> > - for (uint32_t i = 0; i < state->subpass->color_count; ++i) {
> > -
> > - /* The attachment should be one of the attachments described in
> the
> > - * render pass and used in the subpass.
> > - */
> > - const uint32_t a = state->subpass->color_
> attachments[i].attachment;
> > - if (a == VK_ATTACHMENT_UNUSED)
> > - continue;
> > -
> > - assert(a < state->pass->attachment_count);
> > -
> > - /* Store some information regarding this attachment. */
> > - const struct anv_attachment_state *att_state =
> &state->attachments[a];
> > - const struct anv_image_view *iview = state->framebuffer->
> attachments[a];
> > - const struct anv_render_pass_attachment *rp_att =
> > - &state->pass->attachments[a];
> > -
> > - if (att_state->aux_usage == ISL_AUX_USAGE_NONE)
> > - continue;
> > -
> > - /* The fast clear state entry must be updated if a fast clear is
> going to
> > - * happen. The surface state must be updated if the clear value
> from a
> > - * prior fast clear may be needed.
> > - */
> > - if (att_state->pending_clear_aspects && att_state->fast_clear) {
> > - /* Update the fast clear state entry. */
> > - genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->color.state,
> > - iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - true /* copy from ss */);
> > -
> > - /* Fast-clears impact whether or not a resolve will be
> necessary. */
> > - if (att_state->clear_color_is_zero) {
> > - /* This image always has the auxiliary buffer enabled. We
> can mark
> > - * the subresource as not needing a resolve because the
> clear color
> > - * will match what's in every RENDER_SURFACE_STATE object
> when it's
> > - * being used for sampling.
> > - */
> > - set_image_fast_clear_state(cmd_buffer, iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - ANV_FAST_CLEAR_DEFAULT_VALUE);
> > - } else {
> > - set_image_fast_clear_state(cmd_buffer, iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - ANV_FAST_CLEAR_ANY);
> > - }
> > - } else if (rp_att->load_op == VK_ATTACHMENT_LOAD_OP_LOAD &&
> > - iview->planes[0].isl.base_level == 0 &&
> > - iview->planes[0].isl.base_array_layer == 0) {
> > - /* The attachment may have been fast-cleared in a previous
> render
> > - * pass and the value is needed now. Update the surface
> state(s).
> > - *
> > - * TODO: Do this only once per render pass instead of every
> subpass.
> > - */
> > - genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->color.state,
> > - iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - false /* copy to ss */);
> > -
> > - if (need_input_attachment_state(rp_att) &&
> > - att_state->input_aux_usage != ISL_AUX_USAGE_NONE) {
> > - genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->input.state,
> > - iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - false /* copy to ss */);
> > - }
> > - }
> > -
> > - /* We assume that if we're starting a subpass, we're going to do
> some
> > - * rendering so we may end up with compressed data.
> > - */
> > - genX(cmd_buffer_mark_image_written)(cmd_buffer, iview->image,
> > - VK_IMAGE_ASPECT_COLOR_BIT,
> > - att_state->aux_usage,
> > - iview->planes[0].isl.base_
> level,
> > - iview->planes[0].isl.base_
> array_layer,
> > - state->framebuffer->layers);
> > - }
> > -}
> > -
> > static void
> > cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
> > uint32_t subpass_id)
> > @@ -3523,15 +3432,6 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> > */
> > cmd_buffer_subpass_transition_layouts(cmd_buffer, false);
> >
> > - /* Update clear values *after* performing automatic layout
> transitions.
> > - * This ensures that transitions from the UNDEFINED layout have had
> a chance
> > - * to populate the clear value buffer with the correct values for the
> > - * LOAD_OP_LOAD loadOp and that the fast-clears will update the
> buffer
> > - * without the aforementioned layout transition overwriting the
> fast-clear
> > - * value.
> > - */
> > - cmd_buffer_subpass_sync_fast_clear_values(cmd_buffer);
> > -
> > VkRect2D render_area = cmd_buffer->state.render_area;
> > struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> >
> > @@ -3565,6 +3465,25 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> > 0, 0, 1, ISL_AUX_OP_FAST_CLEAR, false);
> > base_layer++;
> > layer_count--;
> > +
> > + genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->color.state,
> > + image,
> VK_IMAGE_ASPECT_COLOR_BIT,
> > + true /* copy from ss */);
> > +
> > + if (att_state->clear_color_is_zero) {
> > + /* This image always has the auxiliary buffer enabled.
> We can
> > + * mark the subresource as not needing a resolve because
> the
> > + * clear color will match what's in every
> RENDER_SURFACE_STATE
> > + * object when it's being used for sampling.
> > + */
>
> This comment is stale. Fast-clearing to zero doesn't imply that the
> image always has the aux buffer enabled.
>
No, it doesn't. However, this is in a block that's predicated on
att_state->fast_clear which does. Looking at the latest version, I've
removed the word "always".
> With that updated, this patch is
> Reviewed-by: Nanley Chery <nanley.g.ch...@intel.com>
>
>
> > + set_image_fast_clear_state(cmd_buffer, iview->image,
> > + VK_IMAGE_ASPECT_COLOR_BIT,
> > + ANV_FAST_CLEAR_DEFAULT_VALUE);
> > + } else {
> > + set_image_fast_clear_state(cmd_buffer, iview->image,
> > + VK_IMAGE_ASPECT_COLOR_BIT,
> > + ANV_FAST_CLEAR_ANY);
> > + }
> > }
> >
> > if (layer_count > 0) {
> > @@ -3605,7 +3524,36 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> > assert(att_state->pending_clear_aspects == 0);
> > }
> >
> > + if (att_state->pending_load_aspects &
> VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) {
> > + if (att_state->aux_usage != ISL_AUX_USAGE_NONE) {
> > + genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->color.state,
> > + image,
> VK_IMAGE_ASPECT_COLOR_BIT,
> > + false /* copy to ss */);
> > + }
> > +
> > + if (need_input_attachment_state(&cmd_state->pass->attachments[a])
> &&
> > + att_state->input_aux_usage != ISL_AUX_USAGE_NONE) {
> > + genX(copy_fast_clear_dwords)(cmd_buffer,
> att_state->input.state,
> > + image,
> VK_IMAGE_ASPECT_COLOR_BIT,
> > + false /* copy to ss */);
> > + }
> > + }
> > +
> > + if (subpass->attachments[i].usage ==
> > + VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT) {
> > + /* We assume that if we're starting a subpass, we're going to
> do some
> > + * rendering so we may end up with compressed data.
> > + */
> > + genX(cmd_buffer_mark_image_written)(cmd_buffer, iview->image,
> > + VK_IMAGE_ASPECT_COLOR_BIT,
> > + att_state->aux_usage,
> > + iview->planes[0].isl.base_
> level,
> > + iview->planes[0].isl.base_
> array_layer,
> > + fb->layers);
> > + }
> > +
> > att_state->pending_clear_aspects = 0;
> > + att_state->pending_load_aspects = 0;
> > }
> >
> > cmd_buffer_emit_depth_stencil(cmd_buffer);
> > --
> > 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
