<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 27, 2018 at 2:22 AM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, Aug 26, 2018 at 11:54 PM Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 08/24/2018 05:04 PM, Jason Ekstrand wrote: > On Fri, Aug 24, 2018 at 12:08 AM Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> > <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>>> wrote: > > > > On 08/22/2018 05:18 PM, Jason Ekstrand wrote: > > On Tue, Aug 21, 2018 at 3:27 AM Tapani Pälli > <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> > > <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>>>> > wrote: > > > > When adding YUV support, we need to figure out > implementation-defined > > external format identifier. > > > > Signed-off-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> > <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> > > <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>>>> > > --- > > 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? </blockquote><div> </div><div> 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.</div></div></div></blockquote><div> </div><div>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.</div><div> </div><div>--Jason </div></div></div>