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?
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
