On Wed, Sep 26, 2018 at 8:29 AM Tapani Pälli <tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>> wrote:
On 9/26/18 10:46 AM, Jason Ekstrand wrote:
> On Mon, Aug 27, 2018 at 2:22 AM Jason Ekstrand
<ja...@jlekstrand.net <mailto:ja...@jlekstrand.net>
> <mailto:ja...@jlekstrand.net <mailto:ja...@jlekstrand.net>>> wrote:
>
> On Sun, Aug 26, 2018 at 11:54 PM Tapani Pälli
> <tapani.pa...@intel.com <mailto:tapani.pa...@intel.com>
<mailto:tapani.pa...@intel.com <mailto:tapani.pa...@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.pa...@intel.com <mailto:tapani.pa...@intel.com>
<mailto:tapani.pa...@intel.com <mailto:tapani.pa...@intel.com>>
> > <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@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.pa...@intel.com
<mailto:tapani.pa...@intel.com> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>>>
> > > <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>>>>>
> > wrote:
> > >
> > > When adding YUV support, we need to figure out
> > implementation-defined
> > > external format identifier.
> > >
> > > Signed-off-by: Tapani Pälli
> <tapani.pa...@intel.com <mailto:tapani.pa...@intel.com>
<mailto:tapani.pa...@intel.com <mailto:tapani.pa...@intel.com>>
> > <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>>>
> > > <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>
> <mailto:tapani.pa...@intel.com
<mailto:tapani.pa...@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.
