[Mesa-dev] [PATCH 3/8] anv/android: add GetAndroidHardwareBufferPropertiesANDROID
Jason Ekstrand
jason at jlekstrand.net
Wed Sep 26 07:46:05 UTC 2018
On Mon, Aug 27, 2018 at 2:22 AM Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Sun, Aug 26, 2018 at 11:54 PM Tapani Pälli <tapani.palli at intel.com>
> wrote:
>
>>
>>
>> On 08/24/2018 05:04 PM, Jason Ekstrand wrote:
>> > On Fri, Aug 24, 2018 at 12:08 AM Tapani Pälli <tapani.palli at intel.com
>> > <mailto:tapani.palli at intel.com>> wrote:
>> >
>> >
>> >
>> > On 08/22/2018 05:18 PM, Jason Ekstrand wrote:
>> > > On Tue, Aug 21, 2018 at 3:27 AM Tapani Pälli
>> > <tapani.palli at intel.com <mailto:tapani.palli at intel.com>
>> > > <mailto:tapani.palli at intel.com <mailto:tapani.palli at intel.com>>>
>> > wrote:
>> > >
>> > > When adding YUV support, we need to figure out
>> > implementation-defined
>> > > external format identifier.
>> > >
>> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com
>> > <mailto:tapani.palli at intel.com>
>> > > <mailto:tapani.palli at intel.com <mailto:
>> tapani.palli at intel.com>>>
>> > > ---
>> > > src/intel/vulkan/anv_android.c | 99
>> > > ++++++++++++++++++++++++++++++++++++++++++
>> > > 1 file changed, 99 insertions(+)
>> > >
>> > > diff --git a/src/intel/vulkan/anv_android.c
>> > > b/src/intel/vulkan/anv_android.c
>> > > index 46c41d57861..7d0eb588e2b 100644
>> > > --- a/src/intel/vulkan/anv_android.c
>> > > +++ b/src/intel/vulkan/anv_android.c
>> > > @@ -29,6 +29,8 @@
>> > > #include <sync/sync.h>
>> > >
>> > > #include "anv_private.h"
>> > > +#include "vk_format_info.h"
>> > > +#include "vk_util.h"
>> > >
>> > > static int anv_hal_open(const struct hw_module_t* mod,
>> > const char*
>> > > id, struct hw_device_t** dev);
>> > > static int anv_hal_close(struct hw_device_t *dev);
>> > > @@ -96,6 +98,103 @@ anv_hal_close(struct hw_device_t *dev)
>> > > return -1;
>> > > }
>> > >
>> > > +static VkResult
>> > > +get_ahw_buffer_format_properties(
>> > > + VkDevice device_h,
>> > > + const struct AHardwareBuffer *buffer,
>> > > + VkAndroidHardwareBufferFormatPropertiesANDROID
>> *pProperties)
>> > > +{
>> > > + ANV_FROM_HANDLE(anv_device, device, device_h);
>> > > +
>> > > + /* Get a description of buffer contents . */
>> > > + AHardwareBuffer_Desc desc;
>> > > + AHardwareBuffer_describe(buffer, &desc);
>> > > +
>> > > + /* Verify description. */
>> > > + uint64_t gpu_usage =
>> > > + AHARDWAREBUFFER_USAGE_GPU_SAMPLED_IMAGE |
>> > > + AHARDWAREBUFFER_USAGE_GPU_COLOR_OUTPUT |
>> > > + AHARDWAREBUFFER_USAGE_GPU_DATA_BUFFER;
>> > > +
>> > > + /* "Buffer must be a valid Android hardware buffer object
>> > with
>> > > at least
>> > > + * one of the AHARDWAREBUFFER_USAGE_GPU_* usage flags."
>> > > + */
>> > > + if (!(desc.usage & (gpu_usage)))
>> > > + return VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR;
>> > > +
>> > > + /* Fill properties fields based on description. */
>> > > + VkAndroidHardwareBufferFormatPropertiesANDROID *p =
>> > pProperties;
>> > > +
>> > > + p->pNext = NULL;
>> > >
>> > >
>> > > You shouldn't be overwriting pNext. That's used by the client to
>> > let
>> > > them chain in multiple structs to fill out in case Google ever
>> > extends
>> > > this extension. Also, while we're here, it'd be good to throw
>> in an
>> > > assert that p->sType is the right thing.
>> >
>> > Yes of course, will remove.
>> >
>> > > + p->format = vk_format_from_android(desc.format);
>> > > + p->externalFormat = 1; /* XXX */
>> > > +
>> > > + const struct anv_format *anv_format =
>> > anv_get_format(p->format);
>> > > + struct anv_physical_device *physical_device =
>> > > + &device->instance->physicalDevice;
>> > > + const struct gen_device_info *devinfo =
>> > &physical_device->info;
>> > >
>> > >
>> > > If all you need is devinfo, that's avilable in the device; you
>> don't
>> > > need to get the physical device for it. Should be device->info.
>> >
>> > OK
>> >
>> > > +
>> > > + p->formatFeatures =
>> > > + anv_get_image_format_features(devinfo, p->format,
>> > > + anv_format,
>> > > VK_IMAGE_TILING_OPTIMAL);
>> > > +
>> > > + /* "The formatFeatures member *must* include
>> > > + * VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT and at least one
>> of
>> > > + * VK_FORMAT_FEATURE_MIDPOINT_CHROMA_SAMPLES_BIT or
>> > > + * VK_FORMAT_FEATURE_COSITED_CHROMA_SAMPLES_BIT"
>> > > + */
>> > > + p->formatFeatures |=
>> > > + VK_FORMAT_FEATURE_MIDPOINT_CHROMA_SAMPLES_BIT;
>> > >
>> > >
>> > > Uh... Why not just throw in SAMPLED_BIT? For that matter, all of
>> > the
>> > > formats you have in your conversion helpers support sampling.
>> Maybe
>> > > just replace that with an assert for now.
>> >
>> > Yeah, VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT is there. Well thing is
>> that
>> > dEQP checks explicitly that either
>> > VK_FORMAT_FEATURE_MIDPOINT_CHROMA_SAMPLES_BIT or
>> > VK_FORMAT_FEATURE_COSITED_CHROMA_SAMPLES_BIT exists (independent of
>> > surface format). TBH I'm not sure what to do about that.
>> >
>> >
>> > That's annoying... Is that in a Khronos CTS test or a Google test?
>> > Either way, we should try to get it corrected. If you don't know how
>> to
>> > do that, I can send some e-mails.
>>
>> But per the Vulkan spec quote above it's not a bug, it says that
>> "formatFeatures member must include sampled bit and at least one of
>> MIDPOINT_CHROMA_SAMPLES or COSITED_CHROMA_SAMPLES. So .. I guess I
>> should limit the supported formats to only those that have
>> anv_format->can_ycbcr() set?
>>
>
> I'll grant that it's consistent with the spec but it also seems like the
> spec has a bug. I'll file a spec bug.
>
I had a chat with Jesse Hall at the F2F in Budapest last week and he
explained why this is needed. The requirement exists so that clients can
always take the external path even if the format is not
VK_FORMAT_UNSUPPORTED. In order to support this, we need to allow color
conversion samplers even for non-YUV formats. It should be fairly easy to
just no-op the format conversion when it's a non-YUV external format. I
don't know if there are tests which exercise the format conversion path on,
say R8G8B8A8_UNORM, but we need to make sure it works.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180926/c1980c81/attachment-0001.html>
More information about the mesa-dev
mailing list