[Mesa-dev] [PATCH 3/8] anv/android: add GetAndroidHardwareBufferPropertiesANDROID
Jason Ekstrand
jason at jlekstrand.net
Wed Sep 26 13:46:34 UTC 2018
On Wed, Sep 26, 2018 at 8:29 AM Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 9/26/18 10:46 AM, Jason Ekstrand wrote:
> > On Mon, Aug 27, 2018 at 2:22 AM Jason Ekstrand <jason at jlekstrand.net
> > <mailto:jason at jlekstrand.net>> wrote:
> >
> > On Sun, Aug 26, 2018 at 11:54 PM Tapani Pälli
> > <tapani.palli at intel.com <mailto: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>
> > > <mailto: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>>
> > > > <mailto: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>>
> > > > <mailto: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.
> >
>
> OK, will try to test this through. It seems currently I'm getting a
> crash in anv_nir_lower_ycbcr_textures() when using sampler created with
> VkSamplerYcbcrConversion together with regular RGB texture, I think
> something is going wrong with component swizzle mapping done there. I'll
> need to verify I'm doing things ok though, I haven't used
> VkSamplerYcbcrConversion previously.
>
Yeah, we just need to detect the case where we have an external format
that's RGBA and no-op it either in lower_ycbcr_conversion or don't create
the conversion object in CreateSampler.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180926/23b9d51c/attachment-0001.html>
More information about the mesa-dev
mailing list