[Mesa-dev] [Mesa-stable] [PATCH] radv: Unset ZRANGE_PRECISION when depth was zeroed

Bas Nieuwenhuizen bas at basnieuwenhuizen.nl
Thu Aug 9 13:33:37 UTC 2018


On Thu, Aug 9, 2018 at 2:56 PM, Andres Gomez <agomez at igalia.com> wrote:
> Bas, James, did you eventually come with a resolution for this? Can I
> just ignore this nominated patch in the -stable ML?

The fix in this patch landed as part of

commit 68dead112e710b261ad33604175d635dec6afd34
Author: Samuel Pitoiset <samuel.pitoiset at gmail.com>
Date:   Wed Jun 13 14:27:40 2018 +0200

    radv: update the ZRANGE_PRECISION value for the TC-compat bug

    On GFX8+, there is a bug that affects TC-compatible depth surfaces
    when the ZRange is not reset after LateZ kills pixels.

    The workaround is to always set DB_Z_INFO.ZRANGE_PRECISION to match
    the last fast clear value. Because the value is set to 1 by default,
    we only need to update it when clearing Z to 0.0.

    We also need to set the depth clear regs and to update
    ZRANGE_PRECISION when initializing a TC-compat depth image to 0.

    Original patch from James Legg.

    This fixes random CTS fails with
    dEQP-VK.renderpass.suballocation.formats.d32_sfloat_s8_uint.input.*

    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105396
    CC: <mesa-stable at lists.freedesktop.org>
    Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
    Reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>


>
> On Wed, 2018-03-28 at 15:28 +0200, Bas Nieuwenhuizen wrote:
>> No final resolution yet.
>>
>> I was trying to fix my minor comment, but looks like I have a bunch of
>> CTS regressions here with the original patch, so still working on it.
>>
>> On Tue, Mar 27, 2018 at 2:03 PM, Juan A. Suarez Romero
>> <jasuarez at igalia.com> wrote:
>> > On Thu, 2018-03-22 at 12:31 +0000, James Legg wrote:
>> > > On Thu, 2018-03-22 at 02:36 +0100, Bas Nieuwenhuizen wrote:
>> > > > On Thu, Mar 8, 2018 at 12:59 PM, James Legg <jlegg at feralinteractive.com> wrote:
>> > > > > This avoids bug 105396 somehow. I suspect it is a VI and GFX9 hardware
>> > > > > bug which PAL calls WaTcCompatZRange, but I don't know for sure.
>> > > > >
>> > > > > In the VK_FORMAT_D32_SFLOAT case, TILE_STENCIL_DISABLE is not set for
>> > > > > tc compatible image formats regardless of not having a stencil aspect.
>> > > > > If TILE_STENCIL_DISABLE was set, ZRANGE_PRECISION would have no effect
>> > > > > and the bug would occur again.
>> > > > >
>> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105396
>> > > > > CC: <mesa-stable at lists.freedesktop.org>
>> > > > > CC: Dave Airlie <airlied at redhat.com>
>> > > > > CC: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>> > > > > CC: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> > > > > ---
>> > > > >  src/amd/vulkan/radv_cmd_buffer.c | 52 +++++++++++++++++++++++++++++++++++++---
>> > > > >  1 file changed, 49 insertions(+), 3 deletions(-)
>> > > > >
>> > > > > diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
>> > > > > index 3e0ed0e9a9..89e31a0347 100644
>> > > > > --- a/src/amd/vulkan/radv_cmd_buffer.c
>> > > > > +++ b/src/amd/vulkan/radv_cmd_buffer.c
>> > > > > @@ -915,6 +915,37 @@ radv_emit_fb_ds_state(struct radv_cmd_buffer *cmd_buffer,
>> > > > >
>> > > > >         }
>> > > > >
>> > > > > +       if (image->surface.htile_size)
>> > > > > +       {
>> > > > > +               /* If the last depth clear value was 0.0f, set ZRANGE_PRECISION
>> > > > > +                * to 0 in dp_z_info for more accuracy with reverse depth; and
>> > > > > +                * to avoid https://bugs.freedesktop.org/show_bug.cgi?id=105396.
>> > > > > +                * Otherwise, we leave it set to 1.
>> > > > > +                */
>> > > > > +               radeon_emit(cmd_buffer->cs, PKT3(PKT3_COND_WRITE, 7, 0));
>> > > > > +
>> > > > > +               const uint32_t write_space = 0 << 8;    /* register */
>> > > > > +               const uint32_t poll_space = 1 << 4;     /* memory */
>> > > > > +               const uint32_t function = 3 << 0;       /* equal to the reference */
>> > > > > +               const uint32_t options = write_space | poll_space | function;
>> > > > > +               radeon_emit(cmd_buffer->cs, options);
>> > > > > +
>> > > > > +               /* poll address - location of the depth clear value */
>> > > > > +               uint64_t va = radv_buffer_get_va(image->bo);
>> > > > > +               va += image->offset + image->clear_value_offset;
>> > > > > +               radeon_emit(cmd_buffer->cs, va);
>> > > > > +               radeon_emit(cmd_buffer->cs, va >> 32);
>> > > > > +
>> > > > > +               radeon_emit(cmd_buffer->cs, fui(0.0f));         /* reference value */
>> > > > > +               radeon_emit(cmd_buffer->cs, (uint32_t)-1);      /* comparison mask */
>> > > > > +               radeon_emit(cmd_buffer->cs, R_028040_DB_Z_INFO >> 2); /* write address low */
>> > > > > +               radeon_emit(cmd_buffer->cs, 0u);                /* write address high */
>> > > > > +
>> > > > > +               /* The value to write data when the condition passes */
>> > > > > +               uint32_t db_z_info_clear_zero = db_z_info & C_028040_ZRANGE_PRECISION;
>> > > > > +               radeon_emit(cmd_buffer->cs, db_z_info_clear_zero);
>> > > > > +       }
>> > > > > +
>> > > > >         radeon_set_context_reg(cmd_buffer->cs, R_028B78_PA_SU_POLY_OFFSET_DB_FMT_CNTL,
>> > > > >                                ds->pa_su_poly_offset_db_fmt_cntl);
>> > > > >  }
>> > > > > @@ -3479,7 +3510,8 @@ void radv_CmdEndRenderPass(
>> > > > >
>> > > > >  /*
>> > > > >   * For HTILE we have the following interesting clear words:
>> > > > > - *   0xfffff30f: Uncompressed, full depth range, for depth+stencil HTILE
>> > > > > + *   0xfffff30f: Uncompressed, full depth range, for depth+stencil HTILE when ZRANGE_PRECISION is 1
>> > > > > + *   0x0003f30f: Uncompressed, full depth range, for depth+stencil HTILE when ZRANGE_PRECISION is 0
>> > > > >   *   0xfffc000f: Uncompressed, full depth range, for depth only HTILE.
>> > > > >   *   0xfffffff0: Clear depth to 1.0
>> > > > >   *   0x00000000: Clear depth to 0.0
>> > > > > @@ -3528,8 +3560,22 @@ static void radv_handle_depth_image_transition(struct radv_cmd_buffer *cmd_buffe
>> > > > >                 radv_initialize_htile(cmd_buffer, image, range, 0);
>> > > > >         } else if (!radv_layout_is_htile_compressed(image, src_layout, src_queue_mask) &&
>> > > > >                    radv_layout_is_htile_compressed(image, dst_layout, dst_queue_mask)) {
>> > > > > -               uint32_t clear_value = vk_format_is_stencil(image->vk_format) ? 0xfffff30f : 0xfffc000f;
>> > > > > -               radv_initialize_htile(cmd_buffer, image, range, clear_value);
>> > > > > +               if (vk_format_is_stencil(image->vk_format)) {
>> > > > > +                       /* The appropriate clear value depends on DB_Z_INFO's
>> > > > > +                        * ZRANGE_PRECISION, which can vary depending on the
>> > > > > +                        * last used clear value, which could be from another
>> > > > > +                        * command buffer. Instead of picking the appropriate
>> > > > > +                        * clear value on the GPU, resummarize accurately.
>> > > > > +                        */
>> > > > > +                       VkImageSubresourceRange local_range = *range;
>> > > > > +                       local_range.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT;
>> > > > > +                       local_range.baseMipLevel = 0;
>> > > > > +                       local_range.levelCount = 1;
>> > > > > +
>> > > > > +                       radv_resummarize_depth_image_inplace(cmd_buffer, image, &local_range);
>> > > >
>> > > > Can we instead of resummarizing, just do the reset + write the clear words?
>> > >
>> > > That would be fine. I used a resummarize as it was already implemented
>> > > and I wasn't confident I would get the clearing implementation right.
>> > >
>> > > > Otherwise looks good to me, please tell if I should do it. I'd do it
>> > > > in a follow up patch, except this is nominated to stable.
>> >
>> > Hi! What was the final resolution for this patch? It was nominated for stable,
>> > but didn't land in master, nor I've seen it merged in a follow up patch.
>> >
>> >
>> >         J.A.
>> >
>> > > If you wouldn't mind, that would be great.
>> > >
>> > > > Apologies for the delay.
>> > >
>> > > No problem.
>> > >
>> > > >
>> > > > > +               } else {
>> > > > > +                       radv_initialize_htile(cmd_buffer, image, range, 0xfffc000f);
>> > > > > +               }
>> > > > >         } else if (radv_layout_is_htile_compressed(image, src_layout, src_queue_mask) &&
>> > > > >                    !radv_layout_is_htile_compressed(image, dst_layout, dst_queue_mask)) {
>> > > > >                 VkImageSubresourceRange local_range = *range;
>> > > > > --
>> > > > > 2.14.3
>> > > > >
>> > >
>> > > _______________________________________________
>> > > mesa-dev mailing list
>> > > mesa-dev at lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>> _______________________________________________
>> mesa-stable mailing list
>> mesa-stable at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
> --
> Br,
>
> Andres


More information about the mesa-dev mailing list