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

[Mauro Rossi]

2017-08-03 20:50 GMT+02:00 Rob Herring <robh at kernel.org>:
> On Thu, Aug 3, 2017 at 11:12 AM, Mauro Rossi <issor.oruam at gmail.com> wrote:
>> 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.
>
> Can't you link against the libmesa_nir (i.e. add to
> LOCAL_STATIC_LIBRARIES) and not add it to GALLIUM_LIBS?

As we speak GALLIUM_LIBS adds LOCAL_STATIC_LIBRARIES,
so a different approach would be required.

>
>>>>
>>> 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
>
> The fix is to either simply sort the whole
> LOCAL_WHOLE_STATIC_LIBRARIES (not just GALLIUM_LIBS) or push the
> libmesa_nir inclusion into the drivers that need it. The latter is the
> better fix IMO. Probably the same thing should be done for
> libmesa_glsl and libmesa_compiler. Additionally, the need for the rest
> of the libs (not in GALLIUM_LIBS) could be moved to
> LOCAL_STATIC_LIBRARIES.
>
> Rob

We can do it with further patch,
I would suggest to continue as Emil said to un-break the build,
with macro that are anyway canonic for Android.

Then we can have (afterwards) a look on the options:

- LOCAL_EXPORT_C_INCLUDE_DIRS in libmesa_nir
- or add  $(dir $(MESA_GEN_NIR_H)) in LOCAL_C_INCLUDES as done for
MESA_GEN_GLSL_H.
- or add LOCAL_GENERATED_SOURCES += MESA_GEN_NIR_H as done in other places

Mauro