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.
>>>> 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.
>>
>>>> + @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
>>>> +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.
>> 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
?
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