[Mesa-dev] [PATCH 3/8] anv/android: add GetAndroidHardwareBufferPropertiesANDROID
Jason Ekstrand
jason at jlekstrand.net
Mon Aug 27 07:22:44 UTC 2018
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.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180827/52d447b2/attachment-0001.html>
More information about the mesa-dev
mailing list