<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 8, 2016 at 3:13 PM, 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 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); </div></div>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? </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - Nanley <div><div class="h5"> > + > + 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-><a href="http://surface_state_block_pool.bo" rel="noreferrer" target="_blank">surface_state_block_pool.bo</a>; > + 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 > </div></div>> _______________________________________________ > 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> </blockquote></div> </div></div>