[Mesa-dev] [PATCH 1/3] android: radeonsi: add nir include paths

[Mauro Rossi]

2017-08-03 16:52 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
> On 3 August 2017 at 13:24, Mauro Rossi <issor.oruam at gmail.com> wrote:
>> 2017-08-03 11:57 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
>>> Hi Mauro,
>>>
>>> Thanks for the series. I'll pull 2&3 in a second - there's a minor
>>> suggestion in this patch.
>>>
>>> On 3 August 2017 at 01:55, Mauro Rossi <issor.oruam at gmail.com> wrote:
>>>> Android build changes to avoid the following building error:
>>>>
>>>> target  C: libmesa_pipe_radeonsi <= external/mesa/src/gallium/drivers/radeonsi/si_pipe.c
>>>> ...
>>>> In file included from external/mesa/src/gallium/drivers/radeonsi/si_pipe.c:38:
>>>> external/mesa/src/compiler/nir/nir.h:48:10: fatal error: 'nir_opcodes.h' file not found
>>>> #include "nir_opcodes.h"
>>>>          ^
>>>> 1 error generated.
>>>>
>>>> Fixes: da62a31c5b "radeonsi: add nir include paths"
>>>> ---
>>>>  src/gallium/drivers/radeonsi/Android.mk | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/gallium/drivers/radeonsi/Android.mk b/src/gallium/drivers/radeonsi/Android.mk
>>>> index 6fff91f6f7..452bba3af8 100644
>>>> --- a/src/gallium/drivers/radeonsi/Android.mk
>>>> +++ b/src/gallium/drivers/radeonsi/Android.mk
>>>> @@ -36,7 +36,8 @@ LOCAL_MODULE_CLASS := STATIC_LIBRARIES
>>>>
>>>>  LOCAL_C_INCLUDES := \
>>>>         $(MESA_TOP)/src/amd/common \
>>>> -       $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_amd_common,,)/common
>>>> +       $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_amd_common,,)/common \
>>>> +       $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir
>>> The more robust solution is to add LOCAL_EXPORT_C_INCLUDE_DIRS for libmesa_nir.
>>>
>>> With that in place we can drop the existing four instances of the
>>> $(call generated-sources-dir-for... libmesa_nir... from the codebase.
>>>
>>> -Emil
>>
>> The current patch proposed is in principle already robust and mimiking
>> automake, but if you prefer other way, I would propose doing this:
>>
>>  LOCAL_C_INCLUDES := \
>> -       $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_amd_common,,)/common
>> +       $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_amd_common,,)/common
>> \
>> +       $(dir $(MESA_GEN_NIR_H))
>>
>> because LOCAL_EXPORT_C_INCLUDE_DIRS in Android.nir.mk would then
>> require static linking of libmesa_nir and we would get double symbols
>> at whole static linking in gallium_dri target due the current
>> dri/target building rules.
>>
> Was under the impression that there'll be symbol problems if both
> places use WHOLE_STATIC.

The issue would happen if libmesa_nir added to GALLIUM_LIBS in
gallium/drivers/radeonsi/Android.mk, after Rob Herring simplifications
to driver building rules and then libmesa_nir being listed twice in
WHOLE_STATIC in gallium/targets/dri/Android.mk

>
>> Is that ok for you?
>>
>> The other places where generated-sourced-dir-for is used for nir could
>> be replaced the same way, but I'd keep it as a separate change if ok
>> for you
>>
>> Please let me know and I'll brew, test build and submit the two pathes
>> to mesa-dev this evening
>>
> Let's go with current approach. It's used in a few places already so
> at least we're consistent.
> I'll push the patch in a moment.
>
> Thanks!
> Emil

Thanks to you
M.