libdrm: intel/Android.mk: Filter libdrm_intel library requirements on x86

When building AOSP after updating libdrm project to the
freedesktop/master branch, I've seen the following build errors:

external/libdrm/intel/Android.mk: error: libdrm_intel
(SHARED_LIBRARIES android-arm64) missing libpciaccess
(SHARED_LIBRARIES android-arm64) You can set
ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
intentional, but that may defer real problems until later in the
build.

Using ALLOW_MISSING_DEPENDENCIES=true when building allows
things to function properly, but is not ideal.

So basically, while I'm not including the libdrm_intel package
into the build, just the fact that the Android.mk file references
libpciaccess which isn't a repo included in AOSP causes the build
failure.

So it seems we need some sort of conditional filter in the
Android.mk to skip over it if we're not building for intel.

This is my initial attempt at solving this.

Feedback would be greatly appreciated!

I note that in the AOSP version of libdrm, the reference to
libpciaccess has been removed. See:
 https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
So I wonder if it make sense to instead remove this upstream as
well?

While this patch addresses upstream's Andorid.mk, I also notice
that the AOSP version of libdrm has converted to Android.bp files:
 https://android.googlesource.com/platform/external/libdrm/+/fa32e29a1fe81e5472aabc65d3aa25a5af5aec55%5E%21/
and wonder if getting that conversion upstream would be a good
idea here?

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Chad Versace <chad.versace@linux.intel.com>
Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Rob Herring <rob.herring@linaro.org>
Cc: Dan Willemsen <dwillemsen@google.com>
Cc: Tomasz Figa <tfiga@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>

---
 intel/Android.mk | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.7.4

On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> When building AOSP after updating libdrm project to the
> freedesktop/master branch, I've seen the following build errors:
>
> external/libdrm/intel/Android.mk: error: libdrm_intel

This is only needed for i915 (not i965) now BTW. I'm not sure at what
point we stop caring about i915.

> (SHARED_LIBRARIES android-arm64) missing libpciaccess
> (SHARED_LIBRARIES android-arm64) You can set
> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
> intentional, but that may defer real problems until later in the
> build.
>
> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
> things to function properly, but is not ideal.
>
> So basically, while I'm not including the libdrm_intel package
> into the build, just the fact that the Android.mk file references
> libpciaccess which isn't a repo included in AOSP causes the build
> failure.
>
> So it seems we need some sort of conditional filter in the
> Android.mk to skip over it if we're not building for intel.
>
> This is my initial attempt at solving this.
>
> Feedback would be greatly appreciated!
>
> I note that in the AOSP version of libdrm, the reference to
> libpciaccess has been removed. See:
>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
> So I wonder if it make sense to instead remove this upstream as
> well?

Only if we drop i915.

>
> While this patch addresses upstream's Andorid.mk, I also notice
> that the AOSP version of libdrm has converted to Android.bp files:
>  https://android.googlesource.com/platform/external/libdrm/+/fa32e29a1fe81e5472aabc65d3aa25a5af5aec55%5E%21/
> and wonder if getting that conversion upstream would be a good
> idea here?

No, because we still support Android versions back to L.

There's also some desire to get meson builds to work in Android build
system so we don't have to support multiple build systems.

>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Chad Versace <chad.versace@linux.intel.com>
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Cc: Dan Willemsen <dwillemsen@google.com>
> Cc: Tomasz Figa <tfiga@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  intel/Android.mk | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/intel/Android.mk b/intel/Android.mk
> index 5407ff3..d834692 100644
> --- a/intel/Android.mk
> +++ b/intel/Android.mk
> @@ -21,6 +21,7 @@
>  # IN THE SOFTWARE.
>  #
>
> +ifeq ($(TARGET_ARCH), x86)

This doesn't work for x86_64. i915 and 64-bit may not be a valid
combination, not sure, but we do at least build test that.

On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>> When building AOSP after updating libdrm project to the
>> freedesktop/master branch, I've seen the following build errors:
>>
>> external/libdrm/intel/Android.mk: error: libdrm_intel
>
> This is only needed for i915 (not i965) now BTW. I'm not sure at what
> point we stop caring about i915.
>
>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>> (SHARED_LIBRARIES android-arm64) You can set
>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>> intentional, but that may defer real problems until later in the
>> build.
>>
>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>> things to function properly, but is not ideal.
>>
>> So basically, while I'm not including the libdrm_intel package
>> into the build, just the fact that the Android.mk file references
>> libpciaccess which isn't a repo included in AOSP causes the build
>> failure.
>>
>> So it seems we need some sort of conditional filter in the
>> Android.mk to skip over it if we're not building for intel.
>>
>> This is my initial attempt at solving this.
>>
>> Feedback would be greatly appreciated!
>>
>> I note that in the AOSP version of libdrm, the reference to
>> libpciaccess has been removed. See:
>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>> So I wonder if it make sense to instead remove this upstream as
>> well?
>
> Only if we drop i915.

To be more precise, drop i915 for Android builds (I'm not suggesting
dropping it elsewhere, just for the Android.mk). I'm really not sure
which devices might be affected. Anyone able to point me to someone in
Intel who would know?


>> +ifeq ($(TARGET_ARCH), x86)
>
> This doesn't work for x86_64. i915 and 64-bit may not be a valid
> combination, not sure, but we do at least build test that.

Out of curiosity, which environment is being used for this build
testing?  Are you describing your generic-build/qemu work or something
else done as part of freedesktop.org?

thanks
-john

On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> When building AOSP after updating libdrm project to the
>>> freedesktop/master branch, I've seen the following build errors:
>>>
>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>
>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>> point we stop caring about i915.
>>
>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>> (SHARED_LIBRARIES android-arm64) You can set
>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>> intentional, but that may defer real problems until later in the
>>> build.
>>>
>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>> things to function properly, but is not ideal.
>>>
>>> So basically, while I'm not including the libdrm_intel package
>>> into the build, just the fact that the Android.mk file references
>>> libpciaccess which isn't a repo included in AOSP causes the build
>>> failure.
>>>
>>> So it seems we need some sort of conditional filter in the
>>> Android.mk to skip over it if we're not building for intel.
>>>
>>> This is my initial attempt at solving this.
>>>
>>> Feedback would be greatly appreciated!
>>>
>>> I note that in the AOSP version of libdrm, the reference to
>>> libpciaccess has been removed. See:
>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>> So I wonder if it make sense to instead remove this upstream as
>>> well?
>>
>> Only if we drop i915.
>
> To be more precise, drop i915 for Android builds (I'm not suggesting
> dropping it elsewhere, just for the Android.mk). I'm really not sure
> which devices might be affected. Anyone able to point me to someone in
> Intel who would know?

The android-x86 folks would be the ones to ask. I added Chih-Wei.

>>> +ifeq ($(TARGET_ARCH), x86)
>>
>> This doesn't work for x86_64. i915 and 64-bit may not be a valid
>> combination, not sure, but we do at least build test that.
>
> Out of curiosity, which environment is being used for this build
> testing?  Are you describing your generic-build/qemu work or something
> else done as part of freedesktop.org?

The CI job I setup to build mesa master (and libdrm implicitly).

Rob

Hi all,

Couple of ideas/notes,

On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> When building AOSP after updating libdrm project to the
>>>> freedesktop/master branch, I've seen the following build errors:
>>>>
>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>
>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>> point we stop caring about i915.
If you're using any other Intel components - say Beignet or the va
driver, I think those still use libdrm_intel.

An alternative solution IMHO, is to drop/tweak the API to not bother
libpciaccess.
I have some ancient cleanup/rework branch

https://github.com/evelikov/libdrm/commits/intel-remove-legacy


>>>
>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>> intentional, but that may defer real problems until later in the
>>>> build.
>>>>
>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>> things to function properly, but is not ideal.
>>>>
>>>> So basically, while I'm not including the libdrm_intel package
>>>> into the build, just the fact that the Android.mk file references
>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>> failure.
>>>>
>>>> So it seems we need some sort of conditional filter in the
>>>> Android.mk to skip over it if we're not building for intel.
>>>>
>>>> This is my initial attempt at solving this.
>>>>
>>>> Feedback would be greatly appreciated!
>>>>
>>>> I note that in the AOSP version of libdrm, the reference to
>>>> libpciaccess has been removed. See:
>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>> So I wonder if it make sense to instead remove this upstream as
>>>> well?
>>>
>>> Only if we drop i915.
>>
>> To be more precise, drop i915 for Android builds (I'm not suggesting
>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>> which devices might be affected. Anyone able to point me to someone in
>> Intel who would know?
>
> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>
A really silly question - how are you triggering any of this if you're
building on !x86?
Is that because the GPU driver is not selected thus you we fall-back
to "build all"?

If so, it might be better to change things to:
 - error out if none selected
 - allow one to select "all", "x86", "arm" and similar groups thus
only the things that can build are build
eg. RobH had fun with x86 intrinsics while building the intel Vulkan
driver on ARM

-Emil

On Fri, Jan 26, 2018 at 10:33 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi all,
>
> Couple of ideas/notes,
>
> On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
>> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> When building AOSP after updating libdrm project to the
>>>>> freedesktop/master branch, I've seen the following build errors:
>>>>>
>>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>>
>>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>>> point we stop caring about i915.
> If you're using any other Intel components - say Beignet or the va
> driver, I think those still use libdrm_intel.
>
> An alternative solution IMHO, is to drop/tweak the API to not bother
> libpciaccess.
> I have some ancient cleanup/rework branch
>
> https://github.com/evelikov/libdrm/commits/intel-remove-legacy

I'm not opposed to this, but I'm really not familiar with intel use
cases and what would be ok or not there.


>>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>>> intentional, but that may defer real problems until later in the
>>>>> build.
>>>>>
>>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>>> things to function properly, but is not ideal.
>>>>>
>>>>> So basically, while I'm not including the libdrm_intel package
>>>>> into the build, just the fact that the Android.mk file references
>>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>>> failure.
>>>>>
>>>>> So it seems we need some sort of conditional filter in the
>>>>> Android.mk to skip over it if we're not building for intel.
>>>>>
>>>>> This is my initial attempt at solving this.
>>>>>
>>>>> Feedback would be greatly appreciated!
>>>>>
>>>>> I note that in the AOSP version of libdrm, the reference to
>>>>> libpciaccess has been removed. See:
>>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>>> So I wonder if it make sense to instead remove this upstream as
>>>>> well?
>>>>
>>>> Only if we drop i915.
>>>
>>> To be more precise, drop i915 for Android builds (I'm not suggesting
>>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>>> which devices might be affected. Anyone able to point me to someone in
>>> Intel who would know?
>>
>> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>>
> A really silly question - how are you triggering any of this if you're
> building on !x86?
> Is that because the GPU driver is not selected thus you we fall-back
> to "build all"?

I think its mostly due the fact we're using the toplevel Android.mk
which includes all Android.mk files in subdirectories.

> If so, it might be better to change things to:
>  - error out if none selected
>  - allow one to select "all", "x86", "arm" and similar groups thus
> only the things that can build are build
> eg. RobH had fun with x86 intrinsics while building the intel Vulkan
> driver on ARM

I'm not sure quite how to select a gpu driver with the Android build
system, other then specifying it via a build variable, which is in
effect what I'm trying to do with this patch.

Other ideas?

Thanks so much for the feedback!
-john

On 30 January 2018 at 05:42, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Jan 26, 2018 at 10:33 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> Hi all,
>>
>> Couple of ideas/notes,
>>
>> On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
>>> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>> When building AOSP after updating libdrm project to the
>>>>>> freedesktop/master branch, I've seen the following build errors:
>>>>>>
>>>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>>>
>>>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>>>> point we stop caring about i915.
>> If you're using any other Intel components - say Beignet or the va
>> driver, I think those still use libdrm_intel.
>>
>> An alternative solution IMHO, is to drop/tweak the API to not bother
>> libpciaccess.
>> I have some ancient cleanup/rework branch
>>
>> https://github.com/evelikov/libdrm/commits/intel-remove-legacy
>
> I'm not opposed to this, but I'm really not familiar with intel use
> cases and what would be ok or not there.
>
>
The unfortunate part is that people familiar don't have to
time/interest to weight in :-(
I might give it another try, one of these days. Unless someone beats me to it.

>>>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>>>> intentional, but that may defer real problems until later in the
>>>>>> build.
>>>>>>
>>>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>>>> things to function properly, but is not ideal.
>>>>>>
>>>>>> So basically, while I'm not including the libdrm_intel package
>>>>>> into the build, just the fact that the Android.mk file references
>>>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>>>> failure.
>>>>>>
>>>>>> So it seems we need some sort of conditional filter in the
>>>>>> Android.mk to skip over it if we're not building for intel.
>>>>>>
>>>>>> This is my initial attempt at solving this.
>>>>>>
>>>>>> Feedback would be greatly appreciated!
>>>>>>
>>>>>> I note that in the AOSP version of libdrm, the reference to
>>>>>> libpciaccess has been removed. See:
>>>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>>>> So I wonder if it make sense to instead remove this upstream as
>>>>>> well?
>>>>>
>>>>> Only if we drop i915.
>>>>
>>>> To be more precise, drop i915 for Android builds (I'm not suggesting
>>>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>>>> which devices might be affected. Anyone able to point me to someone in
>>>> Intel who would know?
>>>
>>> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>>>
>> A really silly question - how are you triggering any of this if you're
>> building on !x86?
>> Is that because the GPU driver is not selected thus you we fall-back
>> to "build all"?
>
> I think its mostly due the fact we're using the toplevel Android.mk
> which includes all Android.mk files in subdirectories.
>
That does not matter. Unless otherwise stated the objects are optional.
Thus they should not be build, unless...

Android changed the policy or you're somehow building stuff that's not required?

>> If so, it might be better to change things to:
>>  - error out if none selected
>>  - allow one to select "all", "x86", "arm" and similar groups thus
>> only the things that can build are build
>> eg. RobH had fun with x86 intrinsics while building the intel Vulkan
>> driver on ARM
>
> I'm not sure quite how to select a gpu driver with the Android build
> system, other then specifying it via a build variable, which is in
> effect what I'm trying to do with this patch.
>
> Other ideas?
>
Based on your input seems like it's not set (grep for
BOARD_GPU_DRIVERS), which results in "everything" being build.
Thus optional libraries (like libdrm_intel) are pulled causing the problem.

My earlier suggestion doesn't sound too crazy, although I'd check with
RobH and the Android-x86 people.
It might require one line change to the device manifest ;-)

Thanks
Emil

On Wed, Jan 31, 2018 at 8:01 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 30 January 2018 at 05:42, John Stultz <john.stultz@linaro.org> wrote:
>> On Fri, Jan 26, 2018 at 10:33 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> Hi all,
>>>
>>> Couple of ideas/notes,
>>>
>>> On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
>>>> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>>>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>>> When building AOSP after updating libdrm project to the
>>>>>>> freedesktop/master branch, I've seen the following build errors:
>>>>>>>
>>>>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>>>>
>>>>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>>>>> point we stop caring about i915.
>>> If you're using any other Intel components - say Beignet or the va
>>> driver, I think those still use libdrm_intel.
>>>
>>> An alternative solution IMHO, is to drop/tweak the API to not bother
>>> libpciaccess.
>>> I have some ancient cleanup/rework branch
>>>
>>> https://github.com/evelikov/libdrm/commits/intel-remove-legacy
>>
>> I'm not opposed to this, but I'm really not familiar with intel use
>> cases and what would be ok or not there.
>>
>>
> The unfortunate part is that people familiar don't have to
> time/interest to weight in :-(
> I might give it another try, one of these days. Unless someone beats me to it.
>
>>>>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>>>>> intentional, but that may defer real problems until later in the
>>>>>>> build.
>>>>>>>
>>>>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>>>>> things to function properly, but is not ideal.
>>>>>>>
>>>>>>> So basically, while I'm not including the libdrm_intel package
>>>>>>> into the build, just the fact that the Android.mk file references
>>>>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>>>>> failure.
>>>>>>>
>>>>>>> So it seems we need some sort of conditional filter in the
>>>>>>> Android.mk to skip over it if we're not building for intel.
>>>>>>>
>>>>>>> This is my initial attempt at solving this.
>>>>>>>
>>>>>>> Feedback would be greatly appreciated!
>>>>>>>
>>>>>>> I note that in the AOSP version of libdrm, the reference to
>>>>>>> libpciaccess has been removed. See:
>>>>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>>>>> So I wonder if it make sense to instead remove this upstream as
>>>>>>> well?
>>>>>>
>>>>>> Only if we drop i915.
>>>>>
>>>>> To be more precise, drop i915 for Android builds (I'm not suggesting
>>>>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>>>>> which devices might be affected. Anyone able to point me to someone in
>>>>> Intel who would know?
>>>>
>>>> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>>>>
>>> A really silly question - how are you triggering any of this if you're
>>> building on !x86?
>>> Is that because the GPU driver is not selected thus you we fall-back
>>> to "build all"?
>>
>> I think its mostly due the fact we're using the toplevel Android.mk
>> which includes all Android.mk files in subdirectories.
>>
> That does not matter. Unless otherwise stated the objects are optional.
> Thus they should not be build, unless...
>
> Android changed the policy or you're somehow building stuff that's not required?

I don't think its a policy, its seems its just how the toplevel
Android.mk file is setup:
   https://cgit.freedesktop.org/drm/libdrm/tree/Android.mk#n63

Where it includes all the Android.mk from all subdirectories, which
pulls in the intel/Android.mk, which adds libpciaccess to the
LOCAL_SHARED_LIBRARIES
   https://cgit.freedesktop.org/drm/libdrm/tree/intel/Android.mk#n36


>> I'm not sure quite how to select a gpu driver with the Android build
>> system, other then specifying it via a build variable, which is in
>> effect what I'm trying to do with this patch.
>>
>> Other ideas?
>>
> Based on your input seems like it's not set (grep for
> BOARD_GPU_DRIVERS), which results in "everything" being build.
> Thus optional libraries (like libdrm_intel) are pulled causing the problem.
>
> My earlier suggestion doesn't sound too crazy, although I'd check with
> RobH and the Android-x86 people.
> It might require one line change to the device manifest ;-)

So I looked a bit at this, but it seems that just controls which
components gets added from the libkms dir, not the top level
directories.

But we could add similar logic to the top level Android.mk file, using
the same BOARD_GPU_DRIVERS value. Hopefully that wouldn't break folks.

Since I'm not sure what else would be a better idea, I'll take a spin
at that, but if you have thoughts on it please do let me know.

Thanks so much for the feedback!
-john

On Wed, Jan 31, 2018 at 12:46 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 31, 2018 at 8:01 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 30 January 2018 at 05:42, John Stultz <john.stultz@linaro.org> wrote:
>>> On Fri, Jan 26, 2018 at 10:33 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> Hi all,
>>>>
>>>> Couple of ideas/notes,
>>>>
>>>> On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
>>>>> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>>>>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>>>> When building AOSP after updating libdrm project to the
>>>>>>>> freedesktop/master branch, I've seen the following build errors:
>>>>>>>>
>>>>>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>>>>>
>>>>>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>>>>>> point we stop caring about i915.
>>>> If you're using any other Intel components - say Beignet or the va
>>>> driver, I think those still use libdrm_intel.
>>>>
>>>> An alternative solution IMHO, is to drop/tweak the API to not bother
>>>> libpciaccess.
>>>> I have some ancient cleanup/rework branch
>>>>
>>>> https://github.com/evelikov/libdrm/commits/intel-remove-legacy
>>>
>>> I'm not opposed to this, but I'm really not familiar with intel use
>>> cases and what would be ok or not there.
>>>
>>>
>> The unfortunate part is that people familiar don't have to
>> time/interest to weight in :-(
>> I might give it another try, one of these days. Unless someone beats me to it.
>>
>>>>>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>>>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>>>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>>>>>> intentional, but that may defer real problems until later in the
>>>>>>>> build.
>>>>>>>>
>>>>>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>>>>>> things to function properly, but is not ideal.
>>>>>>>>
>>>>>>>> So basically, while I'm not including the libdrm_intel package
>>>>>>>> into the build, just the fact that the Android.mk file references
>>>>>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>>>>>> failure.
>>>>>>>>
>>>>>>>> So it seems we need some sort of conditional filter in the
>>>>>>>> Android.mk to skip over it if we're not building for intel.
>>>>>>>>
>>>>>>>> This is my initial attempt at solving this.
>>>>>>>>
>>>>>>>> Feedback would be greatly appreciated!
>>>>>>>>
>>>>>>>> I note that in the AOSP version of libdrm, the reference to
>>>>>>>> libpciaccess has been removed. See:
>>>>>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>>>>>> So I wonder if it make sense to instead remove this upstream as
>>>>>>>> well?
>>>>>>>
>>>>>>> Only if we drop i915.
>>>>>>
>>>>>> To be more precise, drop i915 for Android builds (I'm not suggesting
>>>>>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>>>>>> which devices might be affected. Anyone able to point me to someone in
>>>>>> Intel who would know?
>>>>>
>>>>> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>>>>>
>>>> A really silly question - how are you triggering any of this if you're
>>>> building on !x86?
>>>> Is that because the GPU driver is not selected thus you we fall-back
>>>> to "build all"?
>>>
>>> I think its mostly due the fact we're using the toplevel Android.mk
>>> which includes all Android.mk files in subdirectories.
>>>
>> That does not matter. Unless otherwise stated the objects are optional.
>> Thus they should not be build, unless...
>>
>> Android changed the policy or you're somehow building stuff that's not required?
>
> I don't think its a policy, its seems its just how the toplevel
> Android.mk file is setup:
>    https://cgit.freedesktop.org/drm/libdrm/tree/Android.mk#n63
>
> Where it includes all the Android.mk from all subdirectories, which
> pulls in the intel/Android.mk, which adds libpciaccess to the
> LOCAL_SHARED_LIBRARIES
>    https://cgit.freedesktop.org/drm/libdrm/tree/intel/Android.mk#n36

That's not quite right. The build system descends directories til it
finds an Android.mk. For subdirs under that Android.mk, the Android.mk
file must descend. That's why we have:

include $(call all-makefiles-under,$(LOCAL_PATH))

>>> I'm not sure quite how to select a gpu driver with the Android build
>>> system, other then specifying it via a build variable, which is in
>>> effect what I'm trying to do with this patch.
>>>
>>> Other ideas?
>>>
>> Based on your input seems like it's not set (grep for
>> BOARD_GPU_DRIVERS), which results in "everything" being build.
>> Thus optional libraries (like libdrm_intel) are pulled causing the problem.
>>
>> My earlier suggestion doesn't sound too crazy, although I'd check with
>> RobH and the Android-x86 people.
>> It might require one line change to the device manifest ;-)
>
> So I looked a bit at this, but it seems that just controls which
> components gets added from the libkms dir, not the top level
> directories.
>
> But we could add similar logic to the top level Android.mk file, using
> the same BOARD_GPU_DRIVERS value. Hopefully that wouldn't break folks.

I prefer to not expand the use of BOARD_GPU_DRIVERS and leave it to
dependencies setting what to build.

> Since I'm not sure what else would be a better idea, I'll take a spin
> at that, but if you have thoughts on it please do let me know.

I think we've beat this one to death. Just fix the TARGET_ARCH check
to cover x86 and x86_64. The filter or filter-out make function should
work for this.

Rob

On 1 February 2018 at 14:59, Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 31, 2018 at 12:46 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Jan 31, 2018 at 8:01 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 30 January 2018 at 05:42, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Fri, Jan 26, 2018 at 10:33 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>> Hi all,
>>>>>
>>>>> Couple of ideas/notes,
>>>>>
>>>>> On 10 January 2018 at 20:36, Rob Herring <robh@kernel.org> wrote:
>>>>>> On Wed, Jan 10, 2018 at 1:09 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>>> On Wed, Jan 10, 2018 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
>>>>>>>> On Tue, Jan 9, 2018 at 11:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>>>>> When building AOSP after updating libdrm project to the
>>>>>>>>> freedesktop/master branch, I've seen the following build errors:
>>>>>>>>>
>>>>>>>>> external/libdrm/intel/Android.mk: error: libdrm_intel
>>>>>>>>
>>>>>>>> This is only needed for i915 (not i965) now BTW. I'm not sure at what
>>>>>>>> point we stop caring about i915.
>>>>> If you're using any other Intel components - say Beignet or the va
>>>>> driver, I think those still use libdrm_intel.
>>>>>
>>>>> An alternative solution IMHO, is to drop/tweak the API to not bother
>>>>> libpciaccess.
>>>>> I have some ancient cleanup/rework branch
>>>>>
>>>>> https://github.com/evelikov/libdrm/commits/intel-remove-legacy
>>>>
>>>> I'm not opposed to this, but I'm really not familiar with intel use
>>>> cases and what would be ok or not there.
>>>>
>>>>
>>> The unfortunate part is that people familiar don't have to
>>> time/interest to weight in :-(
>>> I might give it another try, one of these days. Unless someone beats me to it.
>>>
>>>>>>>>> (SHARED_LIBRARIES android-arm64) missing libpciaccess
>>>>>>>>> (SHARED_LIBRARIES android-arm64) You can set
>>>>>>>>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is
>>>>>>>>> intentional, but that may defer real problems until later in the
>>>>>>>>> build.
>>>>>>>>>
>>>>>>>>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows
>>>>>>>>> things to function properly, but is not ideal.
>>>>>>>>>
>>>>>>>>> So basically, while I'm not including the libdrm_intel package
>>>>>>>>> into the build, just the fact that the Android.mk file references
>>>>>>>>> libpciaccess which isn't a repo included in AOSP causes the build
>>>>>>>>> failure.
>>>>>>>>>
>>>>>>>>> So it seems we need some sort of conditional filter in the
>>>>>>>>> Android.mk to skip over it if we're not building for intel.
>>>>>>>>>
>>>>>>>>> This is my initial attempt at solving this.
>>>>>>>>>
>>>>>>>>> Feedback would be greatly appreciated!
>>>>>>>>>
>>>>>>>>> I note that in the AOSP version of libdrm, the reference to
>>>>>>>>> libpciaccess has been removed. See:
>>>>>>>>>  https://android.googlesource.com/platform/external/libdrm/+/f6a1130dffae8de9ddd0c379066daf1df27fc8af%5E%21/
>>>>>>>>> So I wonder if it make sense to instead remove this upstream as
>>>>>>>>> well?
>>>>>>>>
>>>>>>>> Only if we drop i915.
>>>>>>>
>>>>>>> To be more precise, drop i915 for Android builds (I'm not suggesting
>>>>>>> dropping it elsewhere, just for the Android.mk). I'm really not sure
>>>>>>> which devices might be affected. Anyone able to point me to someone in
>>>>>>> Intel who would know?
>>>>>>
>>>>>> The android-x86 folks would be the ones to ask. I added Chih-Wei.
>>>>>>
>>>>> A really silly question - how are you triggering any of this if you're
>>>>> building on !x86?
>>>>> Is that because the GPU driver is not selected thus you we fall-back
>>>>> to "build all"?
>>>>
>>>> I think its mostly due the fact we're using the toplevel Android.mk
>>>> which includes all Android.mk files in subdirectories.
>>>>
>>> That does not matter. Unless otherwise stated the objects are optional.
>>> Thus they should not be build, unless...
>>>
>>> Android changed the policy or you're somehow building stuff that's not required?
>>
>> I don't think its a policy, its seems its just how the toplevel
>> Android.mk file is setup:
>>    https://cgit.freedesktop.org/drm/libdrm/tree/Android.mk#n63
>>
>> Where it includes all the Android.mk from all subdirectories, which
>> pulls in the intel/Android.mk, which adds libpciaccess to the
>> LOCAL_SHARED_LIBRARIES
>>    https://cgit.freedesktop.org/drm/libdrm/tree/intel/Android.mk#n36
>
> That's not quite right. The build system descends directories til it
> finds an Android.mk. For subdirs under that Android.mk, the Android.mk
> file must descend. That's why we have:
>
> include $(call all-makefiles-under,$(LOCAL_PATH))
>
AFAICT the "include" has nothing to do with what gets build (by default).

This page [1] states that, LOCAL_MODULE_TAGS - it's not set, defaults
to optional.
And anything optional is not build/installed.

[1] https://source.android.com/setup/add-device

>>>> I'm not sure quite how to select a gpu driver with the Android build
>>>> system, other then specifying it via a build variable, which is in
>>>> effect what I'm trying to do with this patch.
>>>>
>>>> Other ideas?
>>>>
>>> Based on your input seems like it's not set (grep for
>>> BOARD_GPU_DRIVERS), which results in "everything" being build.
>>> Thus optional libraries (like libdrm_intel) are pulled causing the problem.
>>>
>>> My earlier suggestion doesn't sound too crazy, although I'd check with
>>> RobH and the Android-x86 people.
>>> It might require one line change to the device manifest ;-)
>>
>> So I looked a bit at this, but it seems that just controls which
>> components gets added from the libkms dir, not the top level
>> directories.
>>
Please grep through the whole tree - namely device manifests, Mesa,
libdrm and gralloc(s).
With my proposed solution:
 - device manifest - say, "x86-generic", "arm-generic" "nouveau",
"x86-only" [etc. but no "all"] sets BOARD_GPU_DRIVERS
 - mesa adds the respective dri module(s) to LOCAL_REQUIRED_MODULES
 - drm/gbm/other gralloc does pretty much the same thing
With the correct (until sorted otherwise) gralloc is pulled from the
device manifest

>> But we could add similar logic to the top level Android.mk file, using
>> the same BOARD_GPU_DRIVERS value. Hopefully that wouldn't break folks.
>
> I prefer to not expand the use of BOARD_GPU_DRIVERS and leave it to
> dependencies setting what to build.
>
That also works. For example the "nouveau" case pulls nouveau_dri.so
and libdrm_nouveau.so
Advantages of the BOARD_GPU_DRIVERS are that:
 - set once, don't care about gralloc/mesa/libdrm/other specifics
 - can have meta groups - the above generic/only ones

>> Since I'm not sure what else would be a better idea, I'll take a spin
>> at that, but if you have thoughts on it please do let me know.
>
> I think we've beat this one to death. Just fix the TARGET_ARCH check
> to cover x86 and x86_64. The filter or filter-out make function should
> work for this.
>
Pardon for being a pest, but can someone explain why/how say
libdrm_intel gets pulled?

Is that because BOARD_GPU_DRIVERS is not set, thus Mesa falls back to
"all dri modules" and that attempts to build i915/i965_dri.so.
With the tatter of which pulling libdrm_intel?

If that's the case, special casing on ARCH is not good, since
foo_dri.so may end up without libdrm_foo.

Thanks
Emil