Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Mon, Jan 7, 2019 at 1:23 PM Tapani Pälli wrote:
>
>
>
> On 1/7/19 1:28 PM, Bas Nieuwenhuizen wrote:
> > On Mon, Jan 7, 2019 at 11:54 AM Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 1/7/19 11:56 AM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli
> > wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli
> > wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> >>> On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> >>> 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
> ---
> 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
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 1/7/19 1:28 PM, Bas Nieuwenhuizen wrote:
On Mon, Jan 7, 2019 at 11:54 AM Tapani Pälli wrote:
On 1/7/19 11:56 AM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli wrote:
On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli wrote:
On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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)
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Mon, Jan 7, 2019 at 11:54 AM Tapani Pälli wrote:
>
>
>
> On 1/7/19 11:56 AM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli
> > wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> > On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> > 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
> >> ---
> >> 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
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 1/7/19 11:56 AM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli wrote:
On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli wrote:
On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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?
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,
};
@@
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Wed, Dec 5, 2018 at 1:05 PM Tapani Pälli wrote:
>
>
>
> On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli
> > wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> >>> On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> >>> 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
> ---
> 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?
> >>>
>
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 12/5/18 2:00 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli wrote:
On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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!
(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,
-
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Wed, Dec 5, 2018 at 12:51 PM Tapani Pälli wrote:
>
>
>
> On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> >>> On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli
> >>> wrote:
>
>
>
> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> > On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> > 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
> >> ---
> >> 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
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 12/5/18 1:44 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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?
(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(, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
*grallocUsage |= GRALLOC_USAGE_HW_RENDER;
if (unmask32(, VK_IMAGE_USAGE_TRANSFER_SRC_BIT |
VK_IMAGE_USAGE_SAMPLED_BIT |
+ VK_IMAGE_USAGE_STORAGE_BIT
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Wed, Dec 5, 2018 at 12:37 PM Tapani Pälli wrote:
>
>
>
> On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
> > On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> >>> On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> >>> 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
> ---
> 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,
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 12/5/18 1:22 PM, Bas Nieuwenhuizen wrote:
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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?
(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(, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
*grallocUsage |= GRALLOC_USAGE_HW_RENDER;
if (unmask32(, 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
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Wed, Dec 5, 2018 at 12:15 PM Tapani Pälli wrote:
>
>
>
> On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
> > On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser
> > 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
> >> ---
> >> 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.
>
> > (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(, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
> >>VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
> >> *grallocUsage |= GRALLOC_USAGE_HW_RENDER;
> >>
> >> if (unmask32(, 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
> >>
> >>
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On 12/5/18 1:01 PM, Bas Nieuwenhuizen wrote:
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
---
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.
(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(, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
*grallocUsage |= GRALLOC_USAGE_HW_RENDER;
if (unmask32(, 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
Re: [Mesa-dev] [PATCH] anv/android: handle storage images in vkGetSwapchainGrallocUsageANDROID
On Fri, Sep 7, 2018 at 12:54 AM Kevin Strasser 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
> ---
> 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?)
(Also silently removing the usage bit is bad, because the app could
try actually using images stores with the image ...)
> +
> 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(, VK_IMAGE_USAGE_TRANSFER_DST_BIT |
> VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT))
>*grallocUsage |= GRALLOC_USAGE_HW_RENDER;
>
> if (unmask32(, 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
