On Thu, Jan 11, 2018 at 01:50:41PM -0800, Nanley Chery wrote:
> On Fri, Dec 08, 2017 at 04:16:04PM +0200, Pohjolainen, Topi wrote:
> > On Mon, Nov 27, 2017 at 07:06:07PM -0800, Jason Ekstrand wrote:
> > > This doesn't really change much now but it will give us more/better
> > > control over clears in the future. The one interesting functional
> > > change here is that we are now re-emitting 3DSTATE_DEPTH_BUFFERS and
> > > friends for each clear. However, this only happens at begin_subpass
> > > time so it shouldn't be substantially more expensive.
> > > ---
> > > src/intel/vulkan/anv_blorp.c | 115
> > > +++++++++++--------------------------
> > > src/intel/vulkan/anv_private.h | 8 +++
> > > src/intel/vulkan/genX_cmd_buffer.c | 46 ++++++++++++++-
> > > 3 files changed, 86 insertions(+), 83 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > index 46e2eb0..7401234 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -1138,17 +1138,6 @@ subpass_needs_clear(const struct anv_cmd_buffer
> > > *cmd_buffer)
> > > const struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > > uint32_t ds = cmd_state->subpass->depth_stencil_attachment.attachment;
> > >
> > > - for (uint32_t i = 0; i < cmd_state->subpass->color_count; ++i) {
> > > - uint32_t a = cmd_state->subpass->color_attachments[i].attachment;
> > > - if (a == VK_ATTACHMENT_UNUSED)
> > > - continue;
> > > -
> > > - assert(a < cmd_state->pass->attachment_count);
> > > - if (cmd_state->attachments[a].pending_clear_aspects) {
> > > - return true;
> > > - }
> > > - }
> > > -
> > > if (ds != VK_ATTACHMENT_UNUSED) {
> > > assert(ds < cmd_state->pass->attachment_count);
> > > if (cmd_state->attachments[ds].pending_clear_aspects)
> > > @@ -1182,77 +1171,6 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer
> > > *cmd_buffer)
> > > };
> > >
> > > struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> > > - for (uint32_t i = 0; i < cmd_state->subpass->color_count; ++i) {
> > > - const uint32_t a =
> > > cmd_state->subpass->color_attachments[i].attachment;
> > > - if (a == VK_ATTACHMENT_UNUSED)
> > > - continue;
> > > -
> > > - assert(a < cmd_state->pass->attachment_count);
> > > - struct anv_attachment_state *att_state =
> > > &cmd_state->attachments[a];
> > > -
> > > - if (!att_state->pending_clear_aspects)
> > > - continue;
> > > -
> > > - assert(att_state->pending_clear_aspects ==
> > > VK_IMAGE_ASPECT_COLOR_BIT);
> > > -
> > > - struct anv_image_view *iview = fb->attachments[a];
> > > - const struct anv_image *image = iview->image;
> > > - struct blorp_surf surf;
> > > - get_blorp_surf_for_anv_image(cmd_buffer->device,
> > > - image, VK_IMAGE_ASPECT_COLOR_BIT,
> > > - att_state->aux_usage, &surf);
> > > -
> > > - if (att_state->fast_clear) {
> > > - surf.clear_color =
> > > vk_to_isl_color(att_state->clear_value.color);
> > > -
> > > - /* From the Sky Lake PRM Vol. 7, "Render Target Fast Clear":
> > > - *
> > > - * "After Render target fast clear, pipe-control with color
> > > cache
> > > - * write-flush must be issued before sending any DRAW
> > > commands on
> > > - * that render target."
> > > - *
> > > - * This comment is a bit cryptic and doesn't really tell you
> > > what's
> > > - * going or what's really needed. It appears that fast clear
> > > ops are
> > > - * not properly synchronized with other drawing. This means
> > > that we
> > > - * cannot have a fast clear operation in the pipe at the same
> > > time as
> > > - * other regular drawing operations. We need to use a
> > > PIPE_CONTROL
> > > - * to ensure that the contents of the previous draw hit the
> > > render
> > > - * target before we resolve and then use a second PIPE_CONTROL
> > > after
> > > - * the resolve to ensure that it is completed before any
> > > additional
> > > - * drawing occurs.
> > > - */
> > > - cmd_buffer->state.pending_pipe_bits |=
> > > - ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT |
> > > ANV_PIPE_CS_STALL_BIT;
> > > -
> > > - assert(image->n_planes == 1);
> > > - blorp_fast_clear(&batch, &surf, iview->planes[0].isl.format,
> > > - iview->planes[0].isl.base_level,
> > > - iview->planes[0].isl.base_array_layer,
> > > fb->layers,
> > > - render_area.offset.x, render_area.offset.y,
> > > - render_area.offset.x +
> > > render_area.extent.width,
> > > - render_area.offset.y +
> > > render_area.extent.height);
> > > -
> > > - cmd_buffer->state.pending_pipe_bits |=
> > > - ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT |
> > > ANV_PIPE_CS_STALL_BIT;
> > > - } else {
> > > - assert(image->n_planes == 1);
> > > - anv_cmd_buffer_mark_image_written(cmd_buffer, image,
> > > - VK_IMAGE_ASPECT_COLOR_BIT,
> > > - att_state->aux_usage,
> > > -
> > > iview->planes[0].isl.base_level);
> > > -
> > > - blorp_clear(&batch, &surf, iview->planes[0].isl.format,
> > > -
> > > anv_swizzle_for_render(iview->planes[0].isl.swizzle),
> > > - iview->planes[0].isl.base_level,
> > > - iview->planes[0].isl.base_array_layer, fb->layers,
> > > - render_area.offset.x, render_area.offset.y,
> > > - render_area.offset.x + render_area.extent.width,
> > > - render_area.offset.y + render_area.extent.height,
> > > - vk_to_isl_color(att_state->clear_value.color),
> > > NULL);
> > > - }
> > > -
> > > - att_state->pending_clear_aspects = 0;
> > > - }
> > >
> > > const uint32_t ds =
> > > cmd_state->subpass->depth_stencil_attachment.attachment;
> > > assert(ds == VK_ATTACHMENT_UNUSED || ds <
> > > cmd_state->pass->attachment_count);
> > > @@ -1617,6 +1535,39 @@ isl_to_blorp_hiz_op(enum isl_aux_op isl_op)
> > > }
> > >
> > > void
> > > +anv_image_clear_color(struct anv_cmd_buffer *cmd_buffer,
> > > + const struct anv_image *image,
> > > + VkImageAspectFlagBits aspect,
> > > + enum isl_aux_usage aux_usage,
> > > + enum isl_format format, struct isl_swizzle swizzle,
> > > + uint32_t level, uint32_t base_layer, uint32_t
> > > layer_count,
> > > + VkRect2D area, union isl_color_value clear_color)
> > > +{
> > > + assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > +
> > > + /* We don't support planar images with multisampling yet */
> > > + assert(image->n_planes == 1);
> > > +
> > > + struct blorp_batch batch;
> > > + blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer, 0);
> > > +
> > > + struct blorp_surf surf;
> > > + get_blorp_surf_for_anv_image(cmd_buffer->device, image, aspect,
> > > + aux_usage, &surf);
> > > + anv_cmd_buffer_mark_image_written(cmd_buffer, image, aspect,
> > > + aux_usage, level);
> > > +
> > > + blorp_clear(&batch, &surf, format, anv_swizzle_for_render(swizzle),
> > > + level, base_layer, layer_count,
> > > + area.offset.x, area.offset.y,
> > > + area.offset.x + area.extent.width,
> > > + area.offset.y + area.extent.height,
> > > + clear_color, NULL);
> > > +
> > > + blorp_batch_finish(&batch);
> > > +}
> > > +
> > > +void
> > > anv_image_hiz_op(struct anv_cmd_buffer *cmd_buffer,
> > > const struct anv_image *image,
> > > VkImageAspectFlagBits aspect, uint32_t level,
> > > diff --git a/src/intel/vulkan/anv_private.h
> > > b/src/intel/vulkan/anv_private.h
> > > index e875705..bc355bb 100644
> > > --- a/src/intel/vulkan/anv_private.h
> > > +++ b/src/intel/vulkan/anv_private.h
> > > @@ -2537,6 +2537,14 @@ anv_cmd_buffer_mark_image_written(struct
> > > anv_cmd_buffer *cmd_buffer,
> > > unsigned level);
> > >
> > > void
> > > +anv_image_clear_color(struct anv_cmd_buffer *cmd_buffer,
> > > + const struct anv_image *image,
> > > + VkImageAspectFlagBits aspect,
> > > + enum isl_aux_usage aux_usage,
> > > + enum isl_format format, struct isl_swizzle swizzle,
> > > + uint32_t level, uint32_t base_layer, uint32_t
> > > layer_count,
> > > + VkRect2D area, union isl_color_value clear_color);
> > > +void
> > > anv_image_hiz_op(struct anv_cmd_buffer *cmd_buffer,
> > > const struct anv_image *image,
> > > VkImageAspectFlagBits aspect, uint32_t level,
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > index 56036f7..265ae44 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -3140,7 +3140,9 @@ static void
> > > cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
> > > uint32_t subpass_id)
> > > {
> > > - cmd_buffer->state.subpass =
> > > &cmd_buffer->state.pass->subpasses[subpass_id];
> > > + struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > > + struct anv_subpass *subpass = &cmd_state->pass->subpasses[subpass_id];
> > > + cmd_state->subpass = subpass;
> > >
> > > cmd_buffer->state.dirty |= ANV_CMD_DIRTY_RENDER_TARGETS;
> > >
> > > @@ -3172,6 +3174,48 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > */
> > > 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;
> > > + for (uint32_t i = 0; i < subpass->color_count; ++i) {
> > > + const uint32_t a = subpass->color_attachments[i].attachment;
> > > + if (a == VK_ATTACHMENT_UNUSED)
> > > + continue;
> > > +
> > > + assert(a < cmd_state->pass->attachment_count);
> > > + struct anv_attachment_state *att_state =
> > > &cmd_state->attachments[a];
> > > +
> > > + if (!att_state->pending_clear_aspects)
> > > + continue;
> > > +
> > > + assert(att_state->pending_clear_aspects ==
> > > VK_IMAGE_ASPECT_COLOR_BIT);
> > > +
> > > + struct anv_image_view *iview = fb->attachments[a];
> > > + const struct anv_image *image = iview->image;
> > > +
> > > + /* Multi-planar images are not supported as attachments */
> > > + assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > + assert(image->n_planes == 1);
> > > +
> > > + if (att_state->fast_clear) {
> > > + anv_image_ccs_op(cmd_buffer, image, VK_IMAGE_ASPECT_COLOR_BIT,
> > > + iview->planes[0].isl.base_level,
> > > + iview->planes[0].isl.base_array_layer,
> > > + fb->layers,
> > > + ISL_AUX_OP_FAST_CLEAR, false);
> >
> > I didn't spot anything amiss elsewhere. Here I'm a little puzzled though. It
> > looks there is functional change as now the fast clear targets the entire
> > level. Before in anv_cmd_buffer_clear_subpass() the render area was passed
> > to
> > blorp_fast_clear():
> >
> > blorp_fast_clear(&batch, &surf, iview->planes[0].isl.format,
> > iview->planes[0].isl.base_level,
> > iview->planes[0].isl.base_array_layer, fb->layers,
> > render_area.offset.x, render_area.offset.y,
> > render_area.offset.x + render_area.extent.width,
> > render_area.offset.y + render_area.extent.height);
> >
> > Clearing the entire level makes sense to me and I'm more wondering that did
> > we
> > really do partial fast clears before? Or was the render_area in practise
> > covering the entire level.
> >
>
> The render_area was covering the entire level. genX_cmd_buffer.c:298
> starts the comment and code that disables fast clears if the entire
> level is not covered by the render_area.
Ok, thanks for explaining! I thought it must have been something like that.
This is clearer and at least on my behalf
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> > > + } else {
> > > + anv_image_clear_color(cmd_buffer, image,
> > > VK_IMAGE_ASPECT_COLOR_BIT,
> > > + att_state->aux_usage,
> > > + iview->planes[0].isl.format,
> > > + iview->planes[0].isl.swizzle,
> > > + iview->planes[0].isl.base_level,
> > > + iview->planes[0].isl.base_array_layer,
> > > + fb->layers, render_area,
> > > +
> > > vk_to_isl_color(att_state->clear_value.color));
> > > + }
> > > +
> > > + att_state->pending_clear_aspects = 0;
> > > + }
> > > +
> > > cmd_buffer_emit_depth_stencil(cmd_buffer);
> > >
> > > anv_cmd_buffer_clear_subpass(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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
