[Mesa-dev] [PATCH 2/2] android: add vulkan build for intel

Tue Jan 24 10:10:31 UTC 2017

On 01/24/2017 11:18 AM, Tapani Pälli wrote:
>
>
> On 01/20/2017 03:09 PM, Emil Velikov wrote:
>> On 20 January 2017 at 10:12, Tapani Pälli <tapani.palli at intel.com> wrote:
>>> On 01/20/2017 08:26 AM, Tapani Pälli wrote:
>>>> On 01/19/2017 06:09 PM, Emil Velikov wrote:
>>>>>
>>>>> On 19 January 2017 at 07:10, Tapani Pälli <tapani.palli at intel.com>
>>>>> wrote:
>>>>>>
>>>>>> fixes to issues spotted by Emil Velikov:
>>>>>>
>>>>>>    - set ANV_TIMESTAMP corretly
>>>>>>    - fix typo with VULKAN_GEM_FILES
>>>>>>
>>>>>> v2: update to use Makefile.sources under vulkan
>>>>>>     instead of having own
>>>>>>
>>>>>> v3: update to changes to generate from vk.xml
>>>>>>     (commit c7fc310)
>>>>>>
>>>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>>>> ---
>>>>>>
>>>>>> Note, this patch does not equal to 'Android support'. Patch provides
>>>>>> build system
>>>>>> support so that we can start building driver in Android-IA and going
>>>>>> further to
>>>>>> develop required support. In order to trigger build, you need to add
>>>>>> vulkan.mesa_intel
>>>>>> to PRODUCT_PACKAGES list for the build.
>>>>>>
>>>>>>  src/intel/Android.mk        |   1 +
>>>>>>  src/intel/Android.vulkan.mk | 257
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>
>>>>> I'm leaning towards having git mv Android.vulkan.mk vulkan/Android.mk
>>>>>
>>>>> Afaict both area identical from speed POV on Android, and it'll be
>>>>> symmetrical to the Autoconf ones.
>>>>
>>>>
>>>> That is true but all the other mk's are in src/intel so I wanted to
>>>> follow the current way of things. Do you think we should move blorp,
>>>> common, genxml, isl mk's as well?
>>>>
>> Atm blorp, common, genxml and isl are both "sub"makefiles, on autoconf
>> and android.
>> With the Vulkan one being a standalone one since it depends on
>> mesa/drivers/dri/i965. Where possible I'd rather keep things analogous
>> across autoconf <> android <> scons since it gives a nice ref. point
>> when porting things across build systems plus it makes the build bits
>> less overall.
>
> Fair enough I've moved this, will be sending new version.
>
>>>>>>  2 files changed, 258 insertions(+)
>>>>>>  create mode 100644 src/intel/Android.vulkan.mk
>>>>>>
>>>>>> diff --git a/src/intel/Android.mk b/src/intel/Android.mk
>>>>>> index 3d501ab..7cb2bb9 100644
>>>>>> --- a/src/intel/Android.mk
>>>>>> +++ b/src/intel/Android.mk
>>>>>> @@ -29,3 +29,4 @@ include $(LOCAL_PATH)/Android.blorp.mk
>>>>>>  include $(LOCAL_PATH)/Android.common.mk
>>>>>>  include $(LOCAL_PATH)/Android.genxml.mk
>>>>>>  include $(LOCAL_PATH)/Android.isl.mk
>>>>>> +include $(LOCAL_PATH)/Android.vulkan.mk
>>>>>> diff --git a/src/intel/Android.vulkan.mk
>>>>>> b/src/intel/Android.vulkan.mk
>>>>>> new file mode 100644
>>>>>> index 0000000..bd2626d
>>>>>> --- /dev/null
>>>>>> +++ b/src/intel/Android.vulkan.mk
>>>>>> @@ -0,0 +1,257 @@
>>>>>> +# Copyright © 2017 Intel Corporation
>>>>>> +#
>>>>>> +# Permission is hereby granted, free of charge, to any person
>>>>>> obtaining a
>>>>>> +# copy of this software and associated documentation files (the
>>>>>> "Software"),
>>>>>> +# to deal in the Software without restriction, including without
>>>>>> limitation
>>>>>> +# the rights to use, copy, modify, merge, publish, distribute,
>>>>>> sublicense,
>>>>>> +# and/or sell copies of the Software, and to permit persons to
>>>>>> whom the
>>>>>> +# Software is furnished to do so, subject to the following
>>>>>> conditions:
>>>>>> +#
>>>>>> +# The above copyright notice and this permission notice shall be
>>>>>> included
>>>>>> +# in all copies or substantial portions of the Software.
>>>>>> +#
>>>>>> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>>>>> EXPRESS OR
>>>>>> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>>>> MERCHANTABILITY,
>>>>>> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>>>>> SHALL
>>>>>> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>>>>> OR OTHER
>>>>>> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>>>>> ARISING
>>>>>> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>>>>> OTHER
>>>>>> +# DEALINGS IN THE SOFTWARE.
>>>>>> +
>>>>>> +LOCAL_PATH := $(call my-dir)
>>>>>> +
>>>>>> +include $(CLEAR_VARS)
>>>>>> +include $(LOCAL_PATH)/vulkan/Makefile.sources
>>>>>
>>>>> Then this file gets a s|vulkan/|| + all the addprefix disappears.
>>>>>
>>>>>> +
>>>>>> +VULKAN_FILES := $(addprefix vulkan/, $(VULKAN_FILES))
>>>>>> +VULKAN_GEM_FILES := $(addprefix vulkan/, $(VULKAN_GEM_FILES))
>>>>>> +VULKAN_GENERATED_FILES := vulkan/anv_entrypoints.h
>>>>>> vulkan/anv_timestamp.h
>>>>>> +
>>>>>> +GEN7_FILES := $(addprefix vulkan/, $(GEN7_FILES))
>>>>>> +GEN75_FILES := $(addprefix vulkan/, $(GEN75_FILES))
>>>>>> +GEN8_FILES := $(addprefix vulkan/, $(GEN8_FILES))
>>>>>> +GEN9_FILES := $(addprefix vulkan/, $(GEN9_FILES))
>>>>>> +
>>>>>> +VULKAN_SOURCES := \
>>>>>> +       $(VULKAN_GENERATED_FILES) \
>>>>>> +       $(VULKAN_FILES)
>>>>>> +
>>>>>> +VULKAN_HEADERS := \
>>>>>> +       $(MESA_TOP)/include/vulkan/vk_platform.h \
>>>>>> +       $(MESA_TOP)/include/vulkan/vulkan.h \
>>>>>> +       $(MESA_TOP)/include/vulkan/vulkan_intel.h
>>>>>> +
>>>>>> +VULKAN_COMMON_INCLUDES := \
>>>>>> +       $(VULKAN_HEADERS) \
>>>>>> +       $(MESA_TOP)/src/mapi \
>>>>>> +       $(MESA_TOP)/src/gallium/auxiliary \
>>>>>> +       $(MESA_TOP)/src/gallium/include \
>>>>>> +       $(MESA_TOP)/src/mesa \
>>>>>> +       $(MESA_TOP)/src/mesa/drivers/dri/common \
>>>>>> +       $(MESA_TOP)/src/mesa/drivers/dri/i965 \
>>>>>> +       $(MESA_TOP)/src/vulkan/wsi \
>>>>>> +       $(MESA_TOP)/src/intel/vulkan
>>>>>> +
>>>>>> +#
>>>>>> +# libmesa_anv_entrypoints with header and dummy.c
>>>>>> +#
>>>>>> +
>>>>>> +include $(CLEAR_VARS)
>>>>>> +LOCAL_MODULE := libmesa_anv_entrypoints
>>>>>> +LOCAL_MODULE_CLASS := STATIC_LIBRARIES
>>>>>> +
>>>>>> +intermediates := $(call local-generated-sources-dir)
>>>>>> +
>>>>>> +LOCAL_C_INCLUDES := \
>>>>>> +       $(VULKAN_COMMON_INCLUDES) \
>>>>>> +       $(intermediates)/vulkan
>>>>>> +
>>>>>> +LOCAL_GENERATED_SOURCES += $(addprefix $(intermediates)/,
>>>>>> $(VULKAN_GENERATED_FILES))
>>>>>> +LOCAL_GENERATED_SOURCES += $(intermediates)/vulkan/dummy.c
>>>>>> +
>>>>>> +$(intermediates)/vulkan/dummy.c:
>>>>>
>>>>> Memory is failing: Why do we need a dummy.c at all ?
>>>>
>>>>
>>>> This was about duplicate symbols when linking against
>>>> libmesa_anv_entrypoints later so we could not have the actual c here. I
>>>> can't recall more details.
>>>>
>> Just remembered it:
>> Since we require the header (alongside the C file) by multiple targets
>> and having each anv_gen* create its own instance of the header is
>> redundant and/or race prone (not 100% on this one).
>> Thus we create an empty static library which pulls the dependencies
>> appropriately. Adding the anv_entrypoint.c will result in duplicate
>> symbol when folding the multiple anv_gen* (each one will have a copy)
>> into the final binary.
>>
>> Please add a comment with as much or as little that sound reasonable.
>
> ok
>
>>>>
>>>>>> +       @mkdir -p $(dir $@)
>>>>>> +       @echo "Gen Dummy: $(PRIVATE_MODULE) <= $(notdir $(@))"
>>>>>> +       $(hide) touch $@
>>>>>> +
>>>>>> +define generator
>>>>>> +        @mkdir -p $(dir $@)
>>>>>> +        $(hide) cat $(MESA_TOP)/src/vulkan/registry/vk.xml |
>>>>>> $(PRIVATE_SCRIPT) $(PRIVATE_ARGS) > $@
>>>>>> +endef
>>>>>> +
>>>>>> +$(intermediates)/vulkan/anv_entrypoints.h:
>>>>>> $(intermediates)/vulkan/dummy.c
>>>>>> +$(intermediates)/vulkan/anv_entrypoints.h: PRIVATE_SCRIPT :=
>>>>>> $(MESA_PYTHON2) $(LOCAL_PATH)/vulkan/anv_entrypoints_gen.py
>>>>>> +$(intermediates)/vulkan/anv_entrypoints.h: PRIVATE_ARGS := header
>>>>>> +$(intermediates)/vulkan/anv_entrypoints.h:
>>>>>> +       $(call generator)
>>>>>> +
>>>>>
>>>>> Humble poke if we can share generation rules and/or includes as
>>>>> elaborated here [1] ?
>>>>> Definately no a blocker, just _really_ nice to have.
>>>>
>>>>
>>>> You missed [1] .. these are quite small but yeah if sharing between
>>>> some
>>>> other mk is possible then yes we should go for it.
>>>>
>> Seems like I also missed T in "not".
>>
>> On the generation rules:
>> https://lists.freedesktop.org/archives/mesa-dev/2016-June/120341.html
>> Also skim through IGT's lib/Makefile.sources and related files
>>
>> On the includes de-duplication one could use LOCAL_C{PP,}FLAGS as
>> mentioned in the documentation
>> https://developer.android.com/ndk/guides/android_mk.html
>
> After reading these I'm still a bit puzzled what you mean here but let's
> have some IRC time about this?
>
>>>>>> +LOCAL_SHARED_LIBRARIES := libdrm_intel
>>>>>> +
>>>>>
>>>>> Out of curiosity: is the final binary linked against libdrm_intel ? We
>>>>> don't really use it, but Android build dictates that using the above
>>>>> is the correct thing to do... afaict.
>>>>
>>>>
>>>> We need this to have intel_aub.h (included by brw_context.h), otherwise
>>>> build fails and says that 'intel_aub.h' is not found. There might be
>>>> others too but this is the first failure.
>>>>
>> Yes we need the C/CPP flags, but I'd imagine we end up over-linking
>> against libdrm_intel. Not a huge deal either way - was just a
>> question.
>
> ok
>
>>
>>>> Hmm what do you mean my duplication? This is unique rule for generating
>>>> anv_entrypoints.c while we had almost similar one to generate
>>>> anv_entrypoints.h.
>>>>
>> Brain freeze -  thanks for the clarification :-)
>>
>>
>>>>> Do we need the _WHOLE_ version here ? Skimming through the list (and
>>>>> order) shouldn't getting it closer to the autoconf one help you
>>>>> resolve things ?
>>>>> If _WHOLE_ must stay please mention where/what any of the circular
>>>>> dependencies that mandates it.
>>>>
>>>>
>>>> You are right, it looks like it can be removed. I will test this and
>>>> comment back on this if there is some problem removing it.
>>>
>>>
>>> Did not work out, without WHOLE I get following error:
>>>
>>> VulkanHAL: Could not find vk_icdGetInstanceProcAddr symbol. undefined
>>> symbol: vk_icdGetInstanceProcAddr
>>>
>> IMHO the linker should be smart enough to not discard symbols
>> annotated with default visibility. Perhaps something else is defining
>> PUBLIC in a different manner, thus ours (in macros.h) does not kick in
>> ?
>
> It might be the case. I have noticed other slight differences, for
> example 'anv_CreateSwapchainKHR' has HIDDEN visibility in the resulting
> Android binary but has DEFAULT visibility on my regular desktop build,
> probably the reason why dEQP is not so happy on those swapchain tests :)

Doh forget about this, this function was bad example. This symbol (and 
other swapchain functions) is exposed and implemented in Android 
frameworks libvulkan.so.

>
>> Not sure how much one should bother really, but dropping _WHOLE should
>> shave a few KiB off the final binary.
>> It was just an idea, don't read too much into it.
>>
>> Thanks
>> Emil
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev