On Mon, Jan 7, 2019 at 11:54 AM Tapani Pälli <tapani.pa...@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.pa...@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.pa...@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.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. > >>>> > >>>> 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) > > > > 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@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