[Mesa-dev] [PATCH 09/12] Android: push driver build details to driver makefiles

[Rob Herring]

On Tue, May 2, 2017 at 10:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 2 May 2017 at 15:41, Rob Herring <robh at kernel.org> wrote:
>> On Mon, May 1, 2017 at 2:18 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> Hi Rob,
>>>
>>> On 27 April 2017 at 20:43, Rob Herring <robh at kernel.org> wrote:
>>
>> [...]
>>
>>>> diff --git a/src/gallium/Android.mk b/src/gallium/Android.mk
>>>> index 7c6bda68d59f..d591aaf62e6a 100644
>>>> --- a/src/gallium/Android.mk
>>>> +++ b/src/gallium/Android.mk
>>>> @@ -42,7 +42,9 @@ SUBDIRS += winsys/amdgpu/drm winsys/radeon/drm
>>>>  SUBDIRS += winsys/vc4/drm drivers/vc4
>>>>  SUBDIRS += winsys/virgl/drm winsys/virgl/vtest drivers/virgl
>>>>  SUBDIRS += winsys/svga/drm drivers/svga
>>>> +SUBDIRS += state_trackers/dri
>>>>
>>>> -SUBDIRS += state_trackers/dri targets/dri
>>>> +INC_DIRS := $(call all-named-subdir-makefiles,$(SUBDIRS))
>>>> +INC_DIRS += $(call all-named-subdir-makefiles,targets/dri)
>>>>
>>>> -include $(call all-named-subdir-makefiles,$(SUBDIRS))
>>>> +include $(INC_DIRS)
>>> Why do we need the extra INC_DIRS here? If required please add a small
>>> comment or drop it otherwise.
>>
>> To ensure GALLIUM_*LIBS is filled out by all the drivers before
>> targets/dri. The Android build system doesn't let us have 2 includes
>> and the order returned by all-named-subdir-makefiles varies between
>> make and kati. I'll add a comment.
>>
> Guess one really wants to fix make and kati to align, but for the
> moment a workaround in Mesa should be fine.

I think the fundamental problem is $(wildcard) is used which
explicitly does not have any ordering guarantees. It may be possible
to write the macro in terms of $(foreach) to preserve the order.

OTOH, I think it is good to be explicit that we have this ordering dependency.

>>>> diff --git a/src/gallium/drivers/freedreno/Android.mk b/src/gallium/drivers/freedreno/Android.mk
>>>> index 5c97d9ef2906..330e82420426 100644
>>>> --- a/src/gallium/drivers/freedreno/Android.mk
>>>> +++ b/src/gallium/drivers/freedreno/Android.mk
>>>> @@ -48,3 +48,8 @@ LOCAL_MODULE := libmesa_pipe_freedreno
>>>>  include $(LOCAL_PATH)/Android.gen.mk
>>>>  include $(GALLIUM_COMMON_MK)
>>>>  include $(BUILD_STATIC_LIBRARY)
>>>> +
>>>> +ifneq ($(HAVE_GALLIUM_FREEDRENO),)
>>>> +$(eval GALLIUM_LIBS += $(LOCAL_MODULE) libmesa_winsys_freedreno)
>>>> +$(eval GALLIUM_SHARED_LIBS += $(LOCAL_SHARED_LIBRARIES))
>>> Silly moment - why do we need eval here and below?
>>
>> Otherwise we don't get the current value of LOCAL_* here as
>> GALLIUM_*LIBS will get expanded later.
>>
> Indeed, thank you.
>
>>>> diff --git a/src/gallium/winsys/i915/drm/Android.mk b/src/gallium/winsys/i915/drm/Android.mk
>>>> index b38bd8dca06a..bab3e85c5dd0 100644
>>>> --- a/src/gallium/winsys/i915/drm/Android.mk
>>>> +++ b/src/gallium/winsys/i915/drm/Android.mk
>>>> @@ -35,3 +35,7 @@ LOCAL_MODULE := libmesa_winsys_i915
>>>>
>>>>  include $(GALLIUM_COMMON_MK)
>>>>  include $(BUILD_STATIC_LIBRARY)
>>>> +
>>>> +ifneq ($(HAVE_GALLIUM_I915),)
>>>> +$(eval GALLIUM_SHARED_LIBS += $(LOCAL_SHARED_LIBRARIES))
>>>> +endif
>>> Not 100% sure if we need the winsys hunks. Please add small comment
>>> covering why.
>>
>> The winsys is added by the drivers. This is just additional
>> dependencies the winsys has.
>>
> So if I understand you correctly, Android does not link the
> shared/static libraries if the object is a static library.
> Thus one needs to track those again in the final link.

Right. Though if you use the LOCAL_WHOLE_STATIC_LIBRARIES, then all
the static libs are linked.

> That's a bit counter intuitive since it means that despite using
> "LIBRARIES" variable, Android only uses the headers.
> Regardless...

I guess that's right, but that's not really Android specific in that
the final link needs all of the shared libraries.

Rob