<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">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><div>--Jason </div></div></div>