[Mesa-dev] [PATCH v4 01/18] anv: Transition MCS buffers from the undefined layout

[Lionel Landwerlin]

On 28/07/17 17:21, Nanley Chery wrote:
> On Fri, Jul 28, 2017 at 09:28:45AM +0100, Lionel Landwerlin wrote:
>> On 19/07/17 22:21, Nanley Chery wrote:
>>> Cc: <mesa-stable at lists.freedesktop.org>
>>> Suggested-by: Jason Ekstrand <jason at jlekstrand.net>
>>> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
>>> ---
>>>    src/intel/vulkan/anv_blorp.c       |  8 ++++----
>>>    src/intel/vulkan/anv_private.h     |  8 ++++----
>>>    src/intel/vulkan/genX_cmd_buffer.c | 25 +++++++++++++++----------
>>>    3 files changed, 23 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
>>> index 459d57ec57..d6cbc1c0cd 100644
>>> --- a/src/intel/vulkan/anv_blorp.c
>>> +++ b/src/intel/vulkan/anv_blorp.c
>>> @@ -1434,10 +1434,10 @@ void anv_CmdResolveImage(
>>>    }
>>>    void
>>> -anv_image_ccs_clear(struct anv_cmd_buffer *cmd_buffer,
>>> -                    const struct anv_image *image,
>>> -                    const uint32_t base_level, const uint32_t level_count,
>>> -                    const uint32_t base_layer, uint32_t layer_count)
>>> +anv_image_fast_clear(struct anv_cmd_buffer *cmd_buffer,
>>> +                     const struct anv_image *image,
>>> +                     const uint32_t base_level, const uint32_t level_count,
>>> +                     const uint32_t base_layer, uint32_t layer_count)
>>>    {
>>>       assert(image->type == VK_IMAGE_TYPE_3D || image->extent.depth == 1);
>>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
>>> index 4dce360c76..9a5d2d6fa4 100644
>>> --- a/src/intel/vulkan/anv_private.h
>>> +++ b/src/intel/vulkan/anv_private.h
>>> @@ -2108,10 +2108,10 @@ anv_ccs_resolve(struct anv_cmd_buffer * const cmd_buffer,
>>>                    const enum blorp_fast_clear_op op);
>>>    void
>>> -anv_image_ccs_clear(struct anv_cmd_buffer *cmd_buffer,
>>> -                    const struct anv_image *image,
>>> -                    const uint32_t base_level, const uint32_t level_count,
>>> -                    const uint32_t base_layer, uint32_t layer_count);
>>> +anv_image_fast_clear(struct anv_cmd_buffer *cmd_buffer,
>>> +                     const struct anv_image *image,
>>> +                     const uint32_t base_level, const uint32_t level_count,
>>> +                     const uint32_t base_layer, uint32_t layer_count);
>>>    enum isl_aux_usage
>>>    anv_layout_to_aux_usage(const struct gen_device_info * const devinfo,
>>> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
>>> index 9b3bb10164..81972821d1 100644
>>> --- a/src/intel/vulkan/genX_cmd_buffer.c
>>> +++ b/src/intel/vulkan/genX_cmd_buffer.c
>>> @@ -392,7 +392,9 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
>>>                            VkImageLayout initial_layout,
>>>                            VkImageLayout final_layout)
>>>    {
>>> -   if (image->aux_usage != ISL_AUX_USAGE_CCS_E)
>>> +   assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
>>> +
>>> +   if (image->aux_usage == ISL_AUX_USAGE_NONE)
>>>          return;
>> This bit definitely makes sense to me.
>> I've been hitting it on newer work where the anv_image_fast_clear() would
>> assert because there is no auxiliary surface.
>>
>> I'm not familiar with the CCS stuff enough to review the part below though
>> :(
>>
> No problem, and please don't feel obligated to.
>
>> Acked-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>
> Thanks for the Ack, though, this series has already been pushed
> upstream. The assert you mentioned, are you hitting that on current
> master?

Yes, though I have other changes on top, so it might be my fault.
That's funny you mentioned this is already upstream, I can't see the
      if (image->aux_usage == NONE) return;
in the transition_color_buffer function on master.
Has that changed?

>
> Regards,
> Nanley
>
>>>       if (initial_layout != VK_IMAGE_LAYOUT_UNDEFINED &&
>>> @@ -405,15 +407,18 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
>>>          layer_count = anv_minify(image->extent.depth, base_level);
>>>       }
>>> -#if GEN_GEN >= 9
>>> -   /* We're transitioning from an undefined layout so it doesn't really matter
>>> -    * what data ends up in the color buffer.  We do, however, need to ensure
>>> -    * that the CCS has valid data in it.  One easy way to do that is to
>>> -    * fast-clear the specified range.
>>> -    */
>>> -   anv_image_ccs_clear(cmd_buffer, image, base_level, level_count,
>>> -                       base_layer, layer_count);
>>> -#endif
>>> +   if (image->aux_usage == ISL_AUX_USAGE_CCS_E ||
>>> +       image->samples == 2 || image->samples == 8) {
>>> +      /* We're transitioning from an undefined layout so it doesn't really
>>> +       * matter what data ends up in the color buffer. We do, however, need to
>>> +       * ensure that the auxiliary surface is not in an undefined state. This
>>> +       * state is possible for CCS buffers SKL+ and MCS buffers with certain
>>> +       * sample counts. One easy way to get to a valid state is to fast-clear
>>> +       * the specified range.
>>> +       */
>>> +      anv_image_fast_clear(cmd_buffer, image, base_level, level_count,
>>> +                           base_layer, layer_count);
>>> +   }
>>>    }
>>>    /**
>>