On Fri, Jan 12, 2018 at 04:26:44PM -0800, Jason Ekstrand wrote:
> On Wed, Dec 13, 2017 at 11:00 AM, Nanley Chery <nanleych...@gmail.com>
> wrote:
>
> > On Mon, Nov 27, 2017 at 07:05:59PM -0800, Jason Ekstrand wrote:
> > > This moves it to being based on layout_to_aux_usage instead of being
> > > hard-coded based on bits of a priori knowledge of how transitions
> > > interact with layouts. This conceptually simplifies things because
> > > we're now using layout_to_aux_usage and layout_supports_fast_clear to
> > > make resolve decisions so changes to those functions will do what one
> > > expects.
> > >
> > > This fixes a potential bug with window system integration on gen9+ where
> > > we wouldn't do a resolve when transitioning to the PRESENT_SRC layout
> > > because we just assume that everything that handles CCS_E can handle it
> > > all the time. When handing a CCS_E image off to the window system, we
> > > may need to do a full resolve if the window system does not support the
> > > CCS_E modifier. The only reason why this hasn't been a problem yet is
> > > because we don't support modifiers in Vulkan WSI and so we always get X
> > > tiling which implies no CCS on gen9+.
> > > ---
> > > src/intel/vulkan/genX_cmd_buffer.c | 53 +++++++++++++++++++++++++++++-
> > --------
> > > 1 file changed, 41 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > > index be717eb..65cc85d 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -593,6 +593,7 @@ transition_color_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> > > VkImageLayout initial_layout,
> > > VkImageLayout final_layout)
> > > {
> > > + const struct gen_device_info *devinfo = &cmd_buffer->device->info;
> > > /* Validate the inputs. */
> > > assert(cmd_buffer);
> > > assert(image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV);
> > > @@ -733,17 +734,48 @@ transition_color_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> > > VK_IMAGE_LAYOUT_COLOR_
> > ATTACHMENT_OPTIMAL,
> > > final_layout);
> > > }
> > > - } else if (initial_layout != VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL)
> > {
> > > - /* Resolves are only necessary if the subresource may contain
> > blocks
> > > - * fast-cleared to values unsupported in other layouts. This only
> > occurs
> > > - * if the initial layout is COLOR_ATTACHMENT_OPTIMAL.
> > > - */
> > > - return;
> > > - } else if (image->samples > 1) {
> > > - /* MCS buffers don't need resolving. */
> > > return;
> > > }
> > >
> > > + /* If initial aux usage is NONE, there is nothing to resolve */
> > > + enum isl_aux_usage initial_aux_usage =
> > > + anv_layout_to_aux_usage(devinfo, image, aspect, initial_layout);
> > > + if (initial_aux_usage == ISL_AUX_USAGE_NONE)
> > > + return;
> > > +
> > > + enum isl_aux_op resolve_op = ISL_AUX_OP_NONE;
> > > +
> > > + /* If the initial layout supports fast clear but the final one does
> > not,
> > > + * then we need at least a partial resolve.
> > > + */
> > > + if (anv_layout_supports_fast_clear(devinfo, image, aspect,
> > initial_layout) &&
> > > + !anv_layout_supports_fast_clear(devinfo, image, aspect,
> > final_layout))
> > > + resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
> >
> > I find this hunk more readable, when resolve_op is modified less. How
> > about the following?
> > if (anv_layout_supports_fast_clear(devinfo, image, aspect,
> > initial_layout) &&
> > !anv_layout_supports_fast_clear(devinfo, image, aspect,
> > final_layout)) {
> > /* CCS_D only supports full resolves and BLORP will assert on
> > * us if we try to do a partial resolve on a CCS_D surface.
> > */
> > if (initial_aux_usage == ISL_AUX_USAGE_CCS_D) {
> > resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> > } else {
> > resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
> > }
> > }
> >
> > It may even be more readable to have most of these changes in a function
> > called something like color_transition_to_aux_op(). We shouldn't even
> > need a resolve_op variable in that case. Just a thought.
> >
>
> That only handles two of the three cases, unfortunately. I completely
> agree with you that an if-ladder tends to be easier to read than iterative
> assignment but it's tricky to get this one right. Maybe something like
> this:
>
> if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&
> final_aux_usage != ISL_AUX_USAGE_CCS_E) {
> resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> } else if (anv_layout_supports_fast_clear(devinfo, image, aspect,
> initial_layout) &&
> !anv_layout_supports_fast_clear(devinfo, image, aspect,
> final_layout)) {
> if (initial_aux_usage == ISL_AUX_USAGE_CCS_D) {
> resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> } else {
> resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
> }
> } else {
> resolve_op = ISL_AUX_OP_NONE;
> }
>
> Though even that's confusing because it's weird that we can get into the
> second case with CCS_E. If I were writing this as a helper function, it
> would probably look something like this:
>
> get_resolve_op(...)
> {
> /* If the initial usage supports compression but the final usage does
> not, we have to do a full resolve to get rid of any compressed blocks. */
> if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&
> final_aux_usage != ISL_AUX_USAGE_CCS_E)
> return ISL_AUX_OP_FULL_RESOLVE;
>
> /* If the initial layout can support fast clears but the final cannot,
> then we need a partial resolve */
> if (anv_layout_supports_fast_clear(devinfo, image, aspect,
> initial_layout) &&
> !anv_layout_supports_fast_clear(devinfo, image, aspect,
> final_layout)) {
> /* CCS_D only supports full resolves and BLORP will assert on
> * us if we try to do a partial resolve on a CCS_D surface.
> */
> if (initial_aux_usage == ISL_AUX_USAGE_CCS_D) {
> resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> } else {
> resolve_op = ISL_AUX_OP_PARTIAL_RESOLVE;
> }
> }
>
> /* No resolve needed */
> return ISL_AUX_OP_NONE;
> }
>
>
>
>
Either option looks good to me.
> > > +
> > > + enum isl_aux_usage final_aux_usage =
> >
> > With all the equality comparisons going on with initial_aux_usage and
> > final_aux_usage it would be helpful to have them const.
> >
Not a fan of const here? :P
> > > + anv_layout_to_aux_usage(devinfo, image, aspect, final_layout);
> > > + if (initial_aux_usage == ISL_AUX_USAGE_CCS_E &&
> > > + final_aux_usage != ISL_AUX_USAGE_CCS_E)
> > > + resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> > > +
> > > + /* CCS_D only supports full resolves and BLORP will assert on us if
> > we try
> > > + * to do a partial resolve on a CCS_D surface.
> > > + */
> > > + if (resolve_op == ISL_AUX_OP_PARTIAL_RESOLVE &&
> > > + initial_aux_usage == ISL_AUX_USAGE_CCS_D)
> > > + resolve_op = ISL_AUX_OP_FULL_RESOLVE;
> > > +
> > > + if (resolve_op == ISL_AUX_OP_NONE)
> > > + return;
> > > +
> > > + /* Even though the above code can theoretically handle multiple
> > resolve
> > > + * types such as CCS_D -> CCS_E, the predication code below can't.
> > We only
> >
> > I may be missing something obvious, but what kind of resolve is needed
> > for CCS_D -> CCS_E? Also, why can't the predication code handle multiple
> > resolve types?
> >
>
> That was a bad example. I think what I meant was CCS_E -> NONE.
>
>
> > > + * really handle a couple of cases.
> > > + */
> > > + assert(initial_aux_usage == ISL_AUX_USAGE_NONE ||
> > > + final_aux_usage == ISL_AUX_USAGE_NONE ||
> > > + initial_aux_usage == final_aux_usage);
> > > +
> > > /* Perform a resolve to synchronize data between the main and aux
> > buffer.
> > > * Before we begin, we must satisfy the cache flushing requirement
> > specified
> > > * in the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)":
> > > @@ -774,10 +806,7 @@ transition_color_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> > > genX(load_needs_resolve_predicate)(cmd_buffer, image, aspect,
> > level);
> > >
> > > anv_image_ccs_op(cmd_buffer, image, aspect, level,
> > > - base_layer, layer_count,
> > > - image->planes[plane].aux_usage ==
> > ISL_AUX_USAGE_CCS_E ?
> > > - ISL_AUX_OP_PARTIAL_RESOLVE :
> > ISL_AUX_OP_FULL_RESOLVE,
> > > - true);
> > > + base_layer, layer_count, resolve_op, true);
> > >
> >
> > This patch doesn't seem to fix the bug mentioned in the commit message.
> > I think you also need to disable predication if we're CCS_E and doing a
> > FULL_RESOLVE. At this point, the full resolve will only happen if the
> > image has been fast-cleared with a non-zero clear color.
> >
>
> It doesn't fix it by itself, no. That's what the additional aux tracking
> later in the series is for. Maybe I should change the comment to say that
> it's one step in fixing the mentioned bug.
>
>
Yes, please update the comment. The comment currently says:
"This fixes a potential bug with window system integration on gen9+
[...]"
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
