On Wed, May 03, 2017 at 03:39:51PM -0700, Jason Ekstrand wrote:
> On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <nanleych...@gmail.com>
> wrote:
>
> > There's no image layout to represent a full-RT-cleared color attachment.
> > That's one reason we can end up with redundant resolves. Testing has
> > shown that such resolves can measurably hurt performance and that
> > predicating them can avoid the penalty.
> >
> > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com>
> > ---
> > src/intel/vulkan/anv_blorp.c | 3 +-
> > src/intel/vulkan/anv_image.c | 6 +++
> > src/intel/vulkan/genX_cmd_buffer.c | 106 ++++++++++++++++++++++++++++++
> > ++++---
> > 3 files changed, 108 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > index 821d01a077..32f0edf316 100644
> > --- a/src/intel/vulkan/anv_blorp.c
> > +++ b/src/intel/vulkan/anv_blorp.c
> > @@ -1500,7 +1500,8 @@ anv_ccs_resolve(struct anv_cmd_buffer * const
> > cmd_buffer,
> >
> > struct blorp_batch batch;
> > blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer,
> > - BLORP_BATCH_NO_EMIT_DEPTH_STENCIL);
> > + BLORP_BATCH_NO_EMIT_DEPTH_STENCIL |
> > + BLORP_BATCH_PREDICATE_ENABLE);
> >
> > struct blorp_surf surf;
> > get_blorp_surf_for_anv_image(image, VK_IMAGE_ASPECT_COLOR_BIT,
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index 751f2d6026..92ee86dab5 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -208,6 +208,12 @@ anv_fill_ccs_resolve_ss(const struct anv_device *
> > const device,
> > .aux_usage = image->aux_usage ==
> > ISL_AUX_USAGE_NONE ?
> > ISL_AUX_USAGE_CCS_D :
> > image->aux_usage,
> > .mocs = device->default_mocs);
> > +
> > + /* The following dword is used as a flag to represent whether or not
> > this
> > + * CCS subresource needs resolving. We want this to be zero by default,
> > + * which means that a resolve is not necessary.
> > + */
> > + assert(*(uint32_t *)(data + device->isl_dev.ss.addr_offset) == 0);
> >
>
> This seems like kind-of an odd place to put it... Not a problem, just odd.
>
>
I agree. It's logically better off in anv_BindImage. It's mainly here
for convenience (the data pointer is already calculated for us).
> > }
> >
> > /**
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 95729ec8a8..041301290e 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -402,6 +402,82 @@ transition_depth_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> > anv_gen8_hiz_op_resolve(cmd_buffer, image, hiz_op);
> > }
> >
> > +static uint32_t
> > +clear_value_buffer_offset(const struct anv_cmd_buffer * const cmd_buffer,
> > + const struct anv_image * const image,
> > + const uint8_t level)
> > +{
> > + return image->offset +
> > + image->aux_surface.offset + image->aux_surface.isl.size +
> > + cmd_buffer->device->isl_dev.ss.size * level;
> > +}
> > +
> > +#define MI_PREDICATE_SRC0 0x2400
> > +#define MI_PREDICATE_SRC1 0x2408
> > +
> > +enum ccs_resolve_state {
> > + CCS_RESOLVE_NOT_NEEDED,
> > + CCS_RESOLVE_NEEDED,
> > + CCS_RESOLVE_STARTING,
> > +};
> > +
> > +/* Manages the state of an color image subresource to ensure resolves are
> > + * performed properly.
> > + */
> > +static void
> > +genX(set_resolve_state)(struct anv_cmd_buffer * const cmd_buffer,
> > + const struct anv_image * const image,
> > + const uint8_t level,
> > + const enum ccs_resolve_state state)
> > +{
> > + assert(cmd_buffer && image);
> > +
> > + /* The image subresource range must have a color auxiliary buffer. */
> > + assert(anv_image_has_color_aux(image));
> > + assert(level < anv_color_aux_levels(image));
> > +
> > + /* We store the resolve flag in the location of the surface base
> > address
> > + * field for simplicity and because it is initialized to zero when the
> > + * clear value buffer is initialized.
> > + */
> > + const uint32_t resolve_flag_offset =
> > + clear_value_buffer_offset(cmd_buffer, image, level) +
> > + cmd_buffer->device->isl_dev.ss.addr_offset;
> > +
> > + /* An update operation should not overwrite the fast clear value. */
> > + if (cmd_buffer->device->isl_dev.ss.addr_offset <
> > + cmd_buffer->device->isl_dev.ss.clear_value_offset) {
> > + assert(cmd_buffer->device->isl_dev.ss.addr_offset + 4 <=
> > + cmd_buffer->device->isl_dev.ss.clear_value_offset);
> > + }
> > +
> > + if (state != CCS_RESOLVE_STARTING) {
> > + assert(state == CCS_RESOLVE_NEEDED || state ==
> > CCS_RESOLVE_NOT_NEEDED);
> > + /* The HW docs say that there is no way to guarantee the completion
> > of
> > + * the following command. We use it nevertheless because it shows no
> > + * issues in testing is currently being used in the GL driver.
> > + */
> > + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) {
> > + sdi.Address = (struct anv_address) { image->bo,
> > resolve_flag_offset };
> > + sdi.ImmediateData = state == CCS_RESOLVE_NEEDED;
> > + }
> > + } else {
> > + /* Make the pending predicated resolve a no-op if one is not needed.
> > + * predicate = do_resolve = resolve_flag != 0;
> > + */
> > + emit_lri(&cmd_buffer->batch, MI_PREDICATE_SRC1 , 0);
> > + emit_lri(&cmd_buffer->batch, MI_PREDICATE_SRC1 + 4, 0);
> > + emit_lri(&cmd_buffer->batch, MI_PREDICATE_SRC0 , 0);
> > + emit_lrm(&cmd_buffer->batch, MI_PREDICATE_SRC0 + 4,
> > + image->bo, resolve_flag_offset);
> > + anv_batch_emit(&cmd_buffer->batch, GENX(MI_PREDICATE), mip) {
> > + mip.LoadOperation = LOAD_LOADINV;
> > + mip.CombineOperation = COMBINE_SET;
> > + mip.CompareOperation = COMPARE_SRCS_EQUAL;
> > + }
> >
>
> I really wish the MI_PREDICATE were closer to the anv_ccs_resolve. Maybe
> we could just inline this function? If we had a helper to get the
> anv_address of the resolve flag, it wouldn't be bad at all.
>
>
I also wanted it to be closer. I came up with the current implementation
to work around the anv_blorp / per-gen-compilation divide. What do you
have in mind for inlining? I tried to think something up but it doesn't
look very good.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
