<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 27, 2018 at 4:45 PM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.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 Tue, Feb 27, 2018 at 02:58:01PM -0800, Jason Ekstrand wrote: > On Wed, Feb 21, 2018 at 1:45 PM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a> > > wrote: > > On Gen10+, instead of copying the clear color from the state buffer to > the surface state, just use the address of the state buffer in the > surface state directly. This way we can avoid the copy from state buffer > to surface state. > > Signed-off-by: Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>> > --- > src/intel/vulkan/anv_image.c | 19 ++++++++++++++ > src/intel/vulkan/anv_private.h | 5 ++++ > src/intel/vulkan/genX_cmd_buffer.c | 52 ++++++++++++++++++++++++++++++ > +++++--- > 3 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c > index 0dafe03442d..6b7ea32cbb3 100644 > --- a/src/intel/vulkan/anv_image.c > +++ b/src/intel/vulkan/anv_image.c > @@ -1023,6 +1023,15 @@ anv_image_fill_surface_state(struct anv_device > *device, > const uint64_t aux_address = aux_usage == ISL_AUX_USAGE_NONE ? > 0 : (image->planes[plane].bo_offset + aux_surface->offset); > > + bool use_clear_address = false; > + struct anv_address clear_address = { .bo = NULL }; > + state_inout->clear_address = 0; > + if (device->info.gen >= 10 && aux_usage != ISL_AUX_USAGE_NONE && > + aux_usage != ISL_AUX_USAGE_HIZ) { > + clear_address = anv_image_get_clear_color_addr(device, image, > aspect); > + use_clear_address = true; > + } > + > if (view_usage == ISL_SURF_USAGE_STORAGE_BIT && > !(flags & ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY) && > !isl_has_matching_typed_storage_image_format(&device->info, > @@ -1040,6 +1049,7 @@ anv_image_fill_surface_state(struct anv_device > *device, > .mocs = device->default_mocs); > state_inout->address = address, > state_inout->aux_address = 0; > + state_inout->clear_address = 0; > } else { > if (view_usage == ISL_SURF_USAGE_STORAGE_BIT && > !(flags & ANV_IMAGE_VIEW_STATE_STORAGE_WRITE_ONLY)) { > @@ -1113,6 +1123,8 @@ anv_image_fill_surface_state(struct anv_device > *device, > .aux_surf = &aux_surface->isl, > .aux_usage = aux_usage, > .aux_address = aux_address, > + .clear_address = clear_address.offset, > + .use_clear_address = use_clear_address, > .mocs = device->default_mocs, > .x_offset_sa = tile_x_sa, > .y_offset_sa = tile_y_sa); > @@ -1134,6 +1146,13 @@ anv_image_fill_surface_state(struct anv_device > *device, > assert((aux_address & 0xfff) == 0); > assert(aux_address == (*aux_addr_dw & 0xfffff000)); > state_inout->aux_address = *aux_addr_dw; > + > + if (device->info.gen >= 10 && <a href="http://clear_address.bo" rel="noreferrer" target="_blank">clear_address.bo</a>) { > > > Here you use <a href="http://clear_address.bo" rel="noreferrer" target="_blank">clear_address.bo</a> != NULL but above you use use_clear_address. > Probably best to just pick one and stick with it. > > > + uint32_t *clear_addr_dw = state_inout->state.map + > + device->isl_dev.ss.clear_value_offset; > + assert((clear_address.offset & 0x3f) == 0); > + state_inout->clear_address = *clear_addr_dw; > + } > } > > anv_state_flush(device, state_inout->state); > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_ > private.h > index b8c381d2665..5c077987cef 100644 > --- a/src/intel/vulkan/anv_private.h > +++ b/src/intel/vulkan/anv_private.h > @@ -1674,6 +1674,11 @@ struct anv_surface_state { > * bits of this address include extra aux information. > */ > uint64_t aux_address; > + /* Address of the clear color, if any > + * > + * This address is relative to the start of the BO. > + */ > + uint64_t clear_address; > }; > > /** > diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/ > genX_cmd_buffer.c > index 939a795c2b1..b9e1d50cbe3 100644 > --- a/src/intel/vulkan/genX_cmd_buffer.c > +++ b/src/intel/vulkan/genX_cmd_buffer.c > @@ -200,6 +200,16 @@ add_image_view_relocs(struct anv_cmd_buffer > *cmd_buffer, > if (result != VK_SUCCESS) > anv_batch_set_error(&cmd_buffer->batch, result); > } > + > + if (state.clear_address) { > + VkResult result = > + anv_reloc_list_add(&cmd_buffer->surface_relocs, > + &cmd_buffer->pool->alloc, > + state.state.offset + isl_dev->ss.clear_value_ > offset, > > > I'm not sure how comfortable I am with ss.clear_value_offset doing double-duty > for inline clear values and clear value addresses. I suppose it's probably ok > because the only overlap is gen10 and we know it matches there. > > > + image->planes[image_plane].bo, > state.clear_address); > + if (result != VK_SUCCESS) > + anv_batch_set_error(&cmd_buffer->batch, result); > + } > } > > static void > @@ -1056,6 +1066,35 @@ transition_color_buffer(struct anv_cmd_buffer > *cmd_buffer, > ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT | ANV_PIPE_CS_STALL_BIT; > } > > +static void > +update_fast_clear_color(struct anv_cmd_buffer *cmd_buffer, > + const struct anv_attachment_state *att_state, > + const struct anv_image *image) > +{ > + assert(GEN_GEN >= 10); > + assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT); > + > + struct anv_address clear_address = > + anv_image_get_clear_color_addr(cmd_buffer->device, image, > + VK_IMAGE_ASPECT_COLOR_BIT); > + union isl_color_value fast_clear_value; > + memcpy(fast_clear_value.u32, att_state->clear_value.color.uint32, > + sizeof(fast_clear_value.u32)); > > > I think we probably want to break the clear value handling code from > color_attachment_compute_aux_usage into a helper and use that here instead of > the memcpy. That way things are a bit sanitized. </div></div>Hmmm... Reading again this code, I'm trying to remember why I memcpy the clear color from att_state to fast_clear_value. It looks like I could just have used it directly, like: sdi.ImmediateData = att_state->clear_value.color.uint32[i]; So, do you have a preference between this and the copy done in color_attachment_compute_aux_usage? <div class="HOEnZb"><div class="h5"></div></div></blockquote><div> </div><div>I think I'd prefer the copy done in color_attachment_compute_aux_usage because it sanatizes the value and prevents us from writing undefined garbage into the buffer. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > + > + /* Clear values are stored at the same bo as the aux surface, right > + * after the surface. > + */ > + for (int i = 0; i < cmd_buffer->device->isl_dev.ss.clear_value_size / > 4; i++) { > > > This only gets called on gen10+, just make it 4. If it's ever anything else, > we'll have to modify the stuff below since it just pulls from > fast_clear_value.u32 > > > + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) { > + sdi.Address = (struct anv_address) { > + .bo = <a href="http://clear_address.bo" rel="noreferrer" target="_blank">clear_address.bo</a>, > + .offset = clear_address.offset + i * 4, > + }; > + sdi.ImmediateData = fast_clear_value.u32[i]; > + } > + } > +} > + > /** > * Setup anv_cmd_state::attachments for vkCmdBeginRenderPass. > */ > @@ -3403,9 +3442,13 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer > *cmd_buffer, > base_clear_layer++; > clear_layer_count--; > > - genX(copy_fast_clear_dwords)(cmd_buffer, att_state-> > color.state, > - image, VK_IMAGE_ASPECT_COLOR_BIT, > - true /* copy from ss */); > + if (GEN_GEN < 10) { > + genX(copy_fast_clear_dwords)(cmd_buffer, att_state-> > color.state, > + image, > VK_IMAGE_ASPECT_COLOR_BIT, > + true /* copy from ss */); > + } else { > + update_fast_clear_color(cmd_buffer, att_state, image); > + } > > if (att_state->clear_color_is_zero) { > /* This image has the auxiliary buffer enabled. We can mark > the > @@ -3462,7 +3505,8 @@ 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 (GEN_GEN < 10 && > + (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, > -- > 2.14.3 > > _______________________________________________ > 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> > > </div></div></blockquote></div> </div></div>