On Wed, Sep 26, 2018 at 8:29 AM Tapani Pälli <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>> wrote: > > > > On Sun, Aug 26, 2018 at 11:54 PM Tapani Pälli > > <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>>> 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>>>> > > > 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>>>> > > > > --- > > > > 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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
