On Mon, Jan 7, 2019 at 1:23 PM Tapani Pälli <tapani.pa...@intel.com> wrote:
>
>
>
> On 1/7/19 1:28 PM, Bas Nieuwenhuizen wrote:
> > 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)
>
> 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.
As far as I can tell these are in CTS 9.0 R5, which has been released
in the meantime:
https://android.googlesource.com/platform/external/deqp/+log/60b63e41407db7f9dd9f7b40dfd249234ecb9483
https://source.android.com/compatibility/cts/downloads
Thanks!
>
> 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@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
