On Tue, Nov 8, 2016 at 3:13 PM, Nanley Chery <nanleych...@gmail.com> wrote:
> On Sat, Oct 22, 2016 at 10:50:37AM -0700, Jason Ekstrand wrote:
> > This commit moves the allocation and filling out of surface state from
> > CreateImageView time to BeginRenderPass time. Instead of allocating the
> > render target surface state as part of the image view, we allocate it in
> > the command buffer state at the same time that we set up clears. For
> > secondary command buffers, we allocate memory for the surface states in
> > BeginCommandBuffer but don't fill them out; instead, we use our new
> > SOL-based memcpy function to copy the surface states from the primary
> > command buffer. This allows us to handle secondary command buffers
> without
> > the user specifying the framebuffer ahead-of-time.
> > ---
> > src/intel/vulkan/anv_cmd_buffer.c | 56 ----------
> > src/intel/vulkan/anv_image.c | 22 ----
> > src/intel/vulkan/anv_private.h | 24 ++++-
> > src/intel/vulkan/genX_cmd_buffer.c | 204 +++++++++++++++++++++++++++++-
> -------
> > 4 files changed, 180 insertions(+), 126 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> b/src/intel/vulkan/anv_cmd_buffer.c
> > index a652f9a..372030c 100644
> > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > @@ -144,62 +144,6 @@ anv_cmd_state_reset(struct anv_cmd_buffer
> *cmd_buffer)
> > state->gen7.index_buffer = NULL;
> > }
> >
> > -/**
> > - * Setup anv_cmd_state::attachments for vkCmdBeginRenderPass.
> > - */
> > -void
> > -anv_cmd_state_setup_attachments(struct anv_cmd_buffer *cmd_buffer,
> > - const VkRenderPassBeginInfo *info)
> > -{
> > - struct anv_cmd_state *state = &cmd_buffer->state;
> > - ANV_FROM_HANDLE(anv_render_pass, pass, info->renderPass);
> > -
> > - vk_free(&cmd_buffer->pool->alloc, state->attachments);
> > -
> > - if (pass->attachment_count == 0) {
> > - state->attachments = NULL;
> > - return;
> > - }
> > -
> > - state->attachments = vk_alloc(&cmd_buffer->pool->alloc,
> > - pass->attachment_count *
> > - sizeof(state->attachments[0]),
> > - 8, VK_SYSTEM_ALLOCATION_SCOPE_
> OBJECT);
> > - if (state->attachments == NULL) {
> > - /* FIXME: Propagate VK_ERROR_OUT_OF_HOST_MEMORY to
> vkEndCommandBuffer */
> > - abort();
> > - }
> > -
> > - for (uint32_t i = 0; i < pass->attachment_count; ++i) {
> > - struct anv_render_pass_attachment *att = &pass->attachments[i];
> > - VkImageAspectFlags att_aspects = vk_format_aspects(att->format);
> > - VkImageAspectFlags clear_aspects = 0;
> > -
> > - if (att_aspects == VK_IMAGE_ASPECT_COLOR_BIT) {
> > - /* color attachment */
> > - if (att->load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > - clear_aspects |= VK_IMAGE_ASPECT_COLOR_BIT;
> > - }
> > - } else {
> > - /* depthstencil attachment */
> > - if ((att_aspects & VK_IMAGE_ASPECT_DEPTH_BIT) &&
> > - att->load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > - clear_aspects |= VK_IMAGE_ASPECT_DEPTH_BIT;
> > - }
> > - if ((att_aspects & VK_IMAGE_ASPECT_STENCIL_BIT) &&
> > - att->stencil_load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > - clear_aspects |= VK_IMAGE_ASPECT_STENCIL_BIT;
> > - }
> > - }
> > -
> > - state->attachments[i].pending_clear_aspects = clear_aspects;
> > - if (clear_aspects) {
> > - assert(info->clearValueCount > i);
> > - state->attachments[i].clear_value = info->pClearValues[i];
> > - }
> > - }
> > -}
> > -
> > VkResult
> > anv_cmd_buffer_ensure_push_constants_size(struct anv_cmd_buffer
> *cmd_buffer,
> > gl_shader_stage stage,
> uint32_t size)
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index b7c2e99..b014985 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -504,23 +504,6 @@ anv_CreateImageView(VkDevice _device,
> > iview->sampler_surface_state.alloc_size = 0;
> > }
> >
> > - if (image->usage & VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT) {
> > - iview->color_rt_surface_state = alloc_surface_state(device);
> > -
> > - struct isl_view view = iview->isl;
> > - view.usage |= ISL_SURF_USAGE_RENDER_TARGET_BIT;
> > - isl_surf_fill_state(&device->isl_dev,
> > - iview->color_rt_surface_state.map,
> > - .surf = &surface->isl,
> > - .view = &view,
> > - .mocs = device->default_mocs);
> > -
> > - if (!device->info.has_llc)
> > - anv_state_clflush(iview->color_rt_surface_state);
> > - } else {
> > - iview->color_rt_surface_state.alloc_size = 0;
> > - }
> > -
> > /* NOTE: This one needs to go last since it may stomp
> isl_view.format */
> > if (image->usage & VK_IMAGE_USAGE_STORAGE_BIT) {
> > iview->storage_surface_state = alloc_surface_state(device);
> > @@ -565,11 +548,6 @@ anv_DestroyImageView(VkDevice _device, VkImageView
> _iview,
> > ANV_FROM_HANDLE(anv_device, device, _device);
> > ANV_FROM_HANDLE(anv_image_view, iview, _iview);
> >
> > - if (iview->color_rt_surface_state.alloc_size > 0) {
> > - anv_state_pool_free(&device->surface_state_pool,
> > - iview->color_rt_surface_state);
> > - }
> > -
> > if (iview->sampler_surface_state.alloc_size > 0) {
> > anv_state_pool_free(&device->surface_state_pool,
> > iview->sampler_surface_state);
> > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> > index a6611f1..2a98ea1 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1059,6 +1059,8 @@ void anv_dynamic_state_copy(struct
> anv_dynamic_state *dest,
> > * The clear value is valid only if there exists a pending clear.
> > */
> > struct anv_attachment_state {
> > + struct anv_state color_rt_state;
> > +
> > VkImageAspectFlags pending_clear_aspects;
> > VkClearValue clear_value;
> > };
> > @@ -1099,6 +1101,19 @@ struct anv_cmd_state {
> > */
> > struct anv_attachment_state * attachments;
> >
> > + /**
> > + * Surface states for color render targets. These are stored in a
> single
> > + * flat array. For depth-stencil attachments, the surface state is
> simply
> > + * left blank.
> > + */
> > + struct anv_state render_pass_states;
> > +
> > + /**
> > + * A null surface state of the right size to match the framebuffer.
> This
> > + * is one of the states in render_pass_states.
> > + */
> > + struct anv_state null_surface_state;
> > +
> > struct {
> > struct anv_buffer * index_buffer;
> > uint32_t index_type; /**<
> 3DSTATE_INDEX_BUFFER.IndexFormat */
> > @@ -1237,8 +1252,10 @@ void gen8_cmd_buffer_emit_depth_viewport(struct
> anv_cmd_buffer *cmd_buffer,
> > bool depth_clamp_enable);
> > void gen7_cmd_buffer_emit_scissor(struct anv_cmd_buffer *cmd_buffer);
> >
> > -void anv_cmd_state_setup_attachments(struct anv_cmd_buffer *cmd_buffer,
> > - const VkRenderPassBeginInfo *info);
> > +void anv_cmd_buffer_setup_attachments(struct anv_cmd_buffer
> *cmd_buffer,
> > + struct anv_render_pass *pass,
> > + struct anv_framebuffer
> *framebuffer,
> > + const VkClearValue *clear_values);
> >
> > struct anv_state
> > anv_cmd_buffer_push_constants(struct anv_cmd_buffer *cmd_buffer,
> > @@ -1549,9 +1566,6 @@ struct anv_image_view {
> > VkFormat vk_format;
> > VkExtent3D extent; /**< Extent of VkImageViewCreateInfo::baseMipLevel.
> */
> >
> > - /** RENDER_SURFACE_STATE when using image as a color render target.
> */
> > - struct anv_state color_rt_surface_state;
> > -
> > /** RENDER_SURFACE_STATE when using image as a sampler surface. */
> > struct anv_state sampler_surface_state;
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> > index 8734389..78b9bcc 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -25,6 +25,7 @@
> > #include <stdbool.h>
> >
> > #include "anv_private.h"
> > +#include "vk_format_info.h"
> >
> > #include "common/gen_l3_config.h"
> > #include "genxml/gen_macros.h"
> > @@ -150,6 +151,142 @@ genX(cmd_buffer_emit_state_base_address)(struct
> anv_cmd_buffer *cmd_buffer)
> > }
> > }
> >
> > +/**
> > + * Setup anv_cmd_state::attachments for vkCmdBeginRenderPass.
> > + */
> > +static void
> > +genX(cmd_buffer_setup_attachments)(struct anv_cmd_buffer *cmd_buffer,
> > + struct anv_render_pass *pass,
> > + struct anv_framebuffer *framebuffer,
> > + const VkClearValue *clear_values)
> > +{
> > + const struct isl_device *isl_dev = &cmd_buffer->device->isl_dev;
> > + struct anv_cmd_state *state = &cmd_buffer->state;
> > +
> > + vk_free(&cmd_buffer->pool->alloc, state->attachments);
> > +
> > + if (pass->attachment_count == 0) {
> > + state->attachments = NULL;
> > + return;
> > + }
> > +
> > + state->attachments = vk_alloc(&cmd_buffer->pool->alloc,
> > + pass->attachment_count *
> > + sizeof(state->attachments[0]),
> > + 8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> > + if (state->attachments == NULL) {
> > + /* FIXME: Propagate VK_ERROR_OUT_OF_HOST_MEMORY to
> vkEndCommandBuffer */
> > + abort();
> > + }
> > +
> > + bool need_null_state = false;
> > + for (uint32_t s = 0; s < pass->subpass_count; ++s) {
> > + if (pass->subpasses[s].color_count == 0) {
> > + need_null_state = true;
> > + break;
> > + }
> > + }
> > +
> > + unsigned num_states = need_null_state;
> > + for (uint32_t i = 0; i < pass->attachment_count; ++i) {
> > + if (vk_format_is_color(pass->attachments[i].format))
> > + num_states++;
> > + }
> > +
> > + const uint32_t ss_stride = align_u32(isl_dev->ss.size,
> isl_dev->ss.align);
> > + state->render_pass_states =
> > + anv_state_stream_alloc(&cmd_buffer->surface_state_stream,
> > + num_states * ss_stride, isl_dev->ss.align);
> > +
> > + struct anv_state next_state = state->render_pass_states;
> > + next_state.alloc_size = isl_dev->ss.size;
> > +
> > + if (need_null_state) {
> > + state->null_surface_state = next_state;
> > + next_state.offset += ss_stride;
> > + next_state.map += ss_stride;
> > + }
> > +
> > + for (uint32_t i = 0; i < pass->attachment_count; ++i) {
> > + if (vk_format_is_color(pass->attachments[i].format)) {
> > + state->attachments[i].color_rt_state = next_state;
> > + next_state.offset += ss_stride;
> > + next_state.map += ss_stride;
> > + }
> > + }
> > + assert(next_state.offset == state->render_pass_states.offset +
> > + state->render_pass_states.alloc_size);
> > +
> > + if (framebuffer) {
> > + assert(pass->attachment_count == framebuffer->attachment_count);
> > +
> > + if (need_null_state) {
> > + struct GENX(RENDER_SURFACE_STATE) null_ss = {
> > + .SurfaceType = SURFTYPE_NULL,
> > + .SurfaceArray = framebuffer->layers > 0,
> > + .SurfaceFormat = ISL_FORMAT_R8G8B8A8_UNORM,
> > +#if GEN_GEN >= 8
> > + .TileMode = YMAJOR,
> > +#else
> > + .TiledSurface = true,
> > +#endif
> > + .Width = framebuffer->width - 1,
> > + .Height = framebuffer->height - 1,
> > + .Depth = framebuffer->layers - 1,
> > + .RenderTargetViewExtent = framebuffer->layers - 1,
> > + };
> > + GENX(RENDER_SURFACE_STATE_pack)(NULL,
> state->null_surface_state.map,
> > + &null_ss);
> > + }
> > +
> > + for (uint32_t i = 0; i < pass->attachment_count; ++i) {
> > + struct anv_render_pass_attachment *att = &pass->attachments[i];
> > + VkImageAspectFlags att_aspects = vk_format_aspects(att->format)
> ;
> > + VkImageAspectFlags clear_aspects = 0;
> > +
> > + if (att_aspects == VK_IMAGE_ASPECT_COLOR_BIT) {
> > + /* color attachment */
> > + if (att->load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > + clear_aspects |= VK_IMAGE_ASPECT_COLOR_BIT;
> > + }
> > + } else {
> > + /* depthstencil attachment */
> > + if ((att_aspects & VK_IMAGE_ASPECT_DEPTH_BIT) &&
> > + att->load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > + clear_aspects |= VK_IMAGE_ASPECT_DEPTH_BIT;
> > + }
> > + if ((att_aspects & VK_IMAGE_ASPECT_STENCIL_BIT) &&
> > + att->stencil_load_op == VK_ATTACHMENT_LOAD_OP_CLEAR) {
> > + clear_aspects |= VK_IMAGE_ASPECT_STENCIL_BIT;
> > + }
> > + }
> > +
> > + state->attachments[i].pending_clear_aspects = clear_aspects;
> > + if (clear_aspects)
> > + state->attachments[i].clear_value = clear_values[i];
> > +
> > + struct anv_image_view *iview = framebuffer->attachments[i];
> > + assert(iview->image->vk_format == att->format);
>
> This assertion fails when running Dota 2 on your wip/anv-null-fb branch.
> I don't know if this branch is the most up-to-date version of this
> series, but it was the easiest way to test it (I couldn't get the mailing
> list patches to apply with git am). Are you able to reproduce this?
>
That's odd... I haven't seen that and I have run dota2 with CCS on top of
this branch but may be not in a debug build. What are the two formats when
it fails? I suppose it's possible that it's ATTACHMENT_UNUSED or something.
> - Nanley
>
> > +
> > + if (att_aspects == VK_IMAGE_ASPECT_COLOR_BIT) {
> > + struct isl_view view = iview->isl;
> > + view.usage |= ISL_SURF_USAGE_RENDER_TARGET_BIT;
> > + isl_surf_fill_state(isl_dev,
> > + state->attachments[i].color_
> rt_state.map,
> > + .surf = &iview->image->color_surface.
> isl,
> > + .view = &view,
> > + .mocs = cmd_buffer->device->default_
> mocs);
> > +
> > + anv_cmd_buffer_add_surface_state_reloc(cmd_buffer,
> > + state->attachments[i].color_rt_state, iview->bo,
> iview->offset);
> > + }
> > + }
> > +
> > + if (!cmd_buffer->device->info.has_llc)
> > + anv_state_clflush(state->render_pass_states);
> > + }
> > +}
> > +
> > VkResult
> > genX(BeginCommandBuffer)(
> > VkCommandBuffer commandBuffer,
> > @@ -189,6 +326,9 @@ genX(BeginCommandBuffer)(
> > cmd_buffer->state.subpass =
> > &cmd_buffer->state.pass->subpasses[pBeginInfo->
> pInheritanceInfo->subpass];
> >
> > + genX(cmd_buffer_setup_attachments)(cmd_buffer,
> cmd_buffer->state.pass,
> > + NULL, NULL);
> > +
> > cmd_buffer->state.dirty |= ANV_CMD_DIRTY_RENDER_TARGETS;
> > }
> >
> > @@ -232,6 +372,22 @@ genX(CmdExecuteCommands)(
> >
> > assert(secondary->level == VK_COMMAND_BUFFER_LEVEL_SECONDARY);
> >
> > + if (secondary->usage_flags &
> > + VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT) {
> > + /* If we're continuing a render pass from the primary, we need
> to
> > + * copy the surface states for the current subpass into the
> storage
> > + * we allocated for them in BeginCommandBuffer.
> > + */
> > + struct anv_bo *ss_bo = &primary->device->surface_
> state_block_pool.bo;
> > + struct anv_state src_state = primary->state.render_pass_
> states;
> > + struct anv_state dst_state = secondary->state.render_pass_
> states;
> > + assert(src_state.alloc_size == dst_state.alloc_size);
> > +
> > + genX(cmd_buffer_gpu_memcpy)(primary, ss_bo, dst_state.offset,
> > + ss_bo, src_state.offset,
> > + src_state.alloc_size);
> > + }
> > +
> > anv_cmd_buffer_add_secondary(primary, secondary);
> > }
> >
> > @@ -628,43 +784,11 @@ cmd_buffer_alloc_push_constants(struct
> anv_cmd_buffer *cmd_buffer)
> > cmd_buffer->state.push_constants_dirty |=
> VK_SHADER_STAGE_ALL_GRAPHICS;
> > }
> >
> > -static struct anv_state
> > -alloc_null_surface_state(struct anv_cmd_buffer *cmd_buffer,
> > - struct anv_framebuffer *fb)
> > -{
> > - struct anv_state state =
> > - anv_cmd_buffer_alloc_surface_state(cmd_buffer);
> > -
> > - struct GENX(RENDER_SURFACE_STATE) null_ss = {
> > - .SurfaceType = SURFTYPE_NULL,
> > - .SurfaceArray = fb->layers > 0,
> > - .SurfaceFormat = ISL_FORMAT_R8G8B8A8_UNORM,
> > -#if GEN_GEN >= 8
> > - .TileMode = YMAJOR,
> > -#else
> > - .TiledSurface = true,
> > -#endif
> > - .Width = fb->width - 1,
> > - .Height = fb->height - 1,
> > - .Depth = fb->layers - 1,
> > - .RenderTargetViewExtent = fb->layers - 1,
> > - };
> > -
> > - GENX(RENDER_SURFACE_STATE_pack)(NULL, state.map, &null_ss);
> > -
> > - if (!cmd_buffer->device->info.has_llc)
> > - anv_state_clflush(state);
> > -
> > - return state;
> > -}
> > -
> > -
> > static VkResult
> > emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
> > gl_shader_stage stage,
> > struct anv_state *bt_state)
> > {
> > - struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> > struct anv_subpass *subpass = cmd_buffer->state.subpass;
> > struct anv_pipeline *pipeline;
> > uint32_t bias, state_offset;
> > @@ -743,17 +867,10 @@ emit_binding_table(struct anv_cmd_buffer
> *cmd_buffer,
> > assert(stage == MESA_SHADER_FRAGMENT);
> > assert(binding->binding == 0);
> > if (binding->index < subpass->color_count) {
> > - const struct anv_image_view *iview =
> > - fb->attachments[subpass->color_attachments[binding->
> index]];
> > -
> > - assert(iview->color_rt_surface_state.alloc_size);
> > - surface_state = iview->color_rt_surface_state;
> > - anv_cmd_buffer_add_surface_state_reloc(cmd_buffer,
> surface_state,
> > - iview->bo,
> iview->offset);
> > + const unsigned att = subpass->color_attachments[
> binding->index];
> > + surface_state = cmd_buffer->state.attachments[
> att].color_rt_state;
> > } else {
> > - /* Null render target */
> > - struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> > - surface_state = alloc_null_surface_state(cmd_buffer, fb);
> > + surface_state = cmd_buffer->state.null_surface_state;
> > }
> >
> > bt_map[bias + s] = surface_state.offset + state_offset;
> > @@ -1837,7 +1954,8 @@ void genX(CmdBeginRenderPass)(
> > cmd_buffer->state.framebuffer = framebuffer;
> > cmd_buffer->state.pass = pass;
> > cmd_buffer->state.render_area = pRenderPassBegin->renderArea;
> > - anv_cmd_state_setup_attachments(cmd_buffer, pRenderPassBegin);
> > + genX(cmd_buffer_setup_attachments)(cmd_buffer, pass, framebuffer,
> > + pRenderPassBegin->pClearValues);
> >
> > genX(flush_pipeline_select_3d)(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
