[Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID

[Bas Nieuwenhuizen]

On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
>
> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli <tapani.palli at intel.com> wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> >>> On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser <kevin.strasser at intel.com> wrote:
> >>>>
> >>>> Android P and earlier expect that the surface supports storage images, and
> >>>> so many of the tests fail when the framework checks for that support. The
> >>>> framework also includes various image format and usage combinations that are
> >>>> invalid for the hardware.
> >>>>
> >>>> Drop the STORAGE restriction from the HAL and whitelist a pair of
> >>>> formats so that existing versions of Android can pass these tests.
> >>>>
> >>>> Fixes:
> >>>>      dEQP-VK.wsi.android.*
> >>>>
> >>>> Signed-off-by: Kevin Strasser <kevin.strasser at intel.com>
> >>>> ---
> >>>>    src/intel/vulkan/anv_android.c | 23 ++++++++++++++---------
> >>>>    1 file changed, 14 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/src/intel/vulkan/anv_android.c b/src/intel/vulkan/anv_android.c
> >>>> index 46c41d5..e2640b8 100644
> >>>> --- a/src/intel/vulkan/anv_android.c
> >>>> +++ b/src/intel/vulkan/anv_android.c
> >>>> @@ -234,7 +234,7 @@ VkResult anv_GetSwapchainGrallocUsageANDROID(
> >>>>       *grallocUsage = 0;
> >>>>       intel_logd("%s: format=%d, usage=0x%x", __func__, format, imageUsage);
> >>>>
> >>>> -   /* WARNING: Android Nougat's libvulkan.so hardcodes the VkImageUsageFlags
> >>>> +   /* WARNING: Android's libvulkan.so hardcodes the VkImageUsageFlags
> >>>>        * returned to applications via VkSurfaceCapabilitiesKHR::supportedUsageFlags.
> >>>>        * The relevant code in libvulkan/swapchain.cpp contains this fun comment:
> >>>>        *
> >>>> @@ -247,7 +247,7 @@ VkResult anv_GetSwapchainGrallocUsageANDROID(
> >>>>        * dEQP-VK.wsi.android.swapchain.*.image_usage to fail.
> >>>>        */
> >>>>
> >>>> -   const VkPhysicalDeviceImageFormatInfo2KHR image_format_info = {
> >>>> +   VkPhysicalDeviceImageFormatInfo2KHR image_format_info = {
> >>>
> >>> Why remove the const here?
> >>>
> >>>>          .sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_IMAGE_FORMAT_INFO_2_KHR,
> >>>>          .format = format,
> >>>>          .type = VK_IMAGE_TYPE_2D,
> >>>> @@ -255,6 +255,17 @@ VkResult anv_GetSwapchainGrallocUsageANDROID(
> >>>>          .usage = imageUsage,
> >>>>       };
> >>>>
> >>>> +   /* Android P and earlier doesn't check if the physical device supports a
> >>>> +    * given format and usage combination before calling this function. Omit the
> >>>> +    * storage requirement to make the tests pass.
> >>>> +    */
> >>>> +#if ANDROID_API_LEVEL <= 28
> >>>> +   if (format == VK_FORMAT_R8G8B8A8_SRGB ||
> >>>> +       format == VK_FORMAT_R5G6B5_UNORM_PACK16) {
> >>>> +      image_format_info.usage &= ~VK_IMAGE_USAGE_STORAGE_BIT;
> >>>> +   }
> >>>> +#endif
> >>>
> >>> I don't think you need this. Per the vulkan spec you can only use an
> >>> format + usage combination for a swapchain if it is supported per
> >>> ImageFormatProperties, using essentially the same check happening
> >>> above. I know CTs has been bad at this, but Vulkan CTS should have
> >>> been fixed for a bit now. (I don't think all the fixes are in Android
> >>> CTS 9.0_r4 yet, maybe the next release?)
> >>
> >> AFAIK the problem here is not about CTS. It's the swapchain
> >> implementation that always requires storage support.
> >
> > Actually swapchain creation has the following valid usage rule:
> >
> > "The implied image creation parameters of the swapchain must be
> > supported as reported by vkGetPhysicalDeviceImageFormatProperties"
> >
> > So since those formats don't support the STORAGE usage bit, that test
> > fails and you are not allowed to create a swapchain with those formats
> > and storage, even if the surface capabiliities expose the STORAGE
> > usage bit in general.
>
> Right ... this stuff was done because comment in the swapchain setting
> the bits seems like maybe it's not thought through:
>
> // TODO(jessehall): I think these are right, but haven't thought hard about
> // it. Do we need to query the driver for support of any of these?

That was from before the spec was changed to add that rule.

>
> >>
> >>> (Also silently removing the usage bit is bad, because the app could
> >>> try actually using images stores with the image ...)
> >>
> >> True, it is not nice ..
> >>
> >>
> >>>> +
> >>>>       VkImageFormatProperties2KHR image_format_props = {
> >>>>          .sType = VK_STRUCTURE_TYPE_IMAGE_FORMAT_PROPERTIES_2_KHR,
> >>>>       };
> >>>> @@ -268,19 +279,13 @@ VkResult anv_GetSwapchainGrallocUsageANDROID(
> >>>>                           "inside %s", __func__);
> >>>>       }
> >>>>
> >>>> -   /* Reject STORAGE here to avoid complexity elsewhere. */
> >>>> -   if (imageUsage & VK_IMAGE_USAGE_STORAGE_BIT) {
> >>>> -      return vk_errorf(device->instance, device, VK_ERROR_FORMAT_NOT_SUPPORTED,
> >>>> -                       "VK_IMAGE_USAGE_STORAGE_BIT unsupported for gralloc "
> >>>> -                       "swapchain");
> >>>> -   }
> >>>> -
> >>>>       if (unmask32(&imageUsage, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
> >>>>                                 VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
> >>>>          *grallocUsage |= GRALLOC_USAGE_HW_RENDER;
> >>>>
> >>>>       if (unmask32(&imageUsage, VK_IMAGE_USAGE_TRANSFER_SRC_BIT |
> >>>>                                 VK_IMAGE_USAGE_SAMPLED_BIT |
> >>>> +                             VK_IMAGE_USAGE_STORAGE_BIT |
> >>>>                                 VK_IMAGE_USAGE_INPUT_ATTACHMENT_BIT))
> >>>>          *grallocUsage |= GRALLOC_USAGE_HW_TEXTURE;
> >>>>
> >>>> --
> >>>> 2.7.4
> >>>>
> >>>> _______________________________________________
> >>>> mesa-dev mailing list
> >>>> mesa-dev at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>> _______________________________________________
> >>> mesa-dev mailing list
> >>> mesa-dev at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>>