[Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
Tapani Pälli
tapani.palli at intel.com
Mon Jan 7 12:23:11 UTC 2019
On 1/7/19 1:28 PM, Bas Nieuwenhuizen wrote:
> On Mon, Jan 7, 2019 at 11:54 AM Tapani Pälli <tapani.palli at intel.com> wrote:
>>
>>
>>
>> On 1/7/19 11:56 AM, Bas Nieuwenhuizen wrote:
>>> On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli <tapani.palli at intel.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
>>>>> On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli <tapani.palli at intel.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
>>>>>>> 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.
>>>>>>
>>>>>> OK if I understand correctly, so should we rather then try to fix those
>>>>>> tests to skip instead of fail?
>>>>>
>>>>> They should be fixed with:
>>>>> https://github.com/KhronosGroup/VK-GL-CTS/commit/49eab80e4a8b3af1790b9ac88b096aa9bffd193f#diff-8369d6640a2c6ad0c0fc1d85b113faeb
>>>>> https://github.com/KhronosGroup/VK-GL-CTS/commit/858f5396a4f63223fcf31f717d23b4b552e10182#diff-8369d6640a2c6ad0c0fc1d85b113faeb
>>>>
>>>> Thanks, will try with these!
>>>
>>> Hi,
>>>
>>> Did you have any luck with this? This patch (or mine) are still
>>> pending review based on this?
>>
>> Sorry I've forgotten this but will get to this now. Could you please
>> pinpoint which patch from you was referred here?
>
> https://patchwork.freedesktop.org/patch/265974/
>
> (Though it is missing a bit: see
> https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/1366537
> for what I ended up using in ChromeOS)
Yep, I've tested it and the CTS patches fix the failures. Do you know if
there is going to be another CTS P release with these changes? Until
that happens, we probably need to carry a WA in Android tree.
Consider this also as rb for your patch (the one used by cros).
>>
>>
>>> Thanks,
>>> Bas
>>>>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> (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
>>>>>>>>>>>
More information about the mesa-dev
mailing list