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

Tue Jan 24 09:18:08 UTC 2017

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 :)

> 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
>