On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli <tapani.pa...@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.pa...@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.stras...@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.stras...@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@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > >>> _______________________________________________ > >>> mesa-dev mailing list > >>> mesa-dev@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > >>> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev