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

[Emil Velikov]

Hi Rob,

On 27 April 2017 at 20:43, Rob Herring <robh at kernel.org> wrote:

> +classic_drivers := i915.HAVE_I915_DRI i965.HAVE_I965_DRI
> +gallium_drivers := \
> +       swrast.HAVE_GALLIUM_SOFTPIPE \
> +       freedreno.HAVE_GALLIUM_FREEDRENO \
> +       i915g.HAVE_GALLIUM_I915 \
> +       nouveau.HAVE_GALLIUM_NOUVEAU \
> +       r300g.HAVE_GALLIUM_R300 \
> +       r600g.HAVE_GALLIUM_R600 \
> +       radeonsi.HAVE_GALLIUM_RADEONSI \
> +       vmwgfx.HAVE_GALLIUM_VMWGFX \
> +       vc4.HAVE_GALLIUM_VC4 \
> +       virgl.HAVE_GALLIUM_VIRGL
>
> -MESA_GPU_DRIVERS := $(strip $(BOARD_GPU_DRIVERS))
> -
> -# warn about invalid drivers
> -invalid_drivers := $(filter-out \
> -       $(classic_drivers) $(gallium_drivers), $(MESA_GPU_DRIVERS))
> -ifneq ($(invalid_drivers),)
> -$(warning invalid GPU drivers: $(invalid_drivers))
> -# tidy up
> -MESA_GPU_DRIVERS := $(filter-out $(invalid_drivers), $(MESA_GPU_DRIVERS))
> -endif
> +MESA_BUILD_CLASSIC := $(strip $(foreach d, $(BOARD_GPU_DRIVERS), $(patsubst $(d).%,%, $(filter $(d).%, $(classic_drivers)))))
> +MESA_BUILD_GALLIUM := $(strip $(foreach d, $(BOARD_GPU_DRIVERS), $(patsubst $(d).%,%, $(filter $(d).%, $(gallium_drivers)))))
> +$(foreach d, $(MESA_BUILD_CLASSIC) $(MESA_BUILD_GALLIUM), $(eval $(d) := true))
>
Can you please add a one line description about the driver.HAVE_FOO pairs.
Be that here or above the declaration {classic,gallium}_drivers above.


> --- a/src/egl/Android.mk
> +++ b/src/egl/Android.mk
> @@ -57,16 +57,10 @@ LOCAL_SHARED_LIBRARIES := \
>         libcutils \
>         libsync
>
> -ifeq ($(strip $(MESA_BUILD_CLASSIC)),true)
> -# require i915_dri and/or i965_dri
> -LOCAL_REQUIRED_MODULES += \
> -       $(addsuffix _dri, $(filter i915 i965, $(MESA_GPU_DRIVERS)))
> -endif # MESA_BUILD_CLASSIC
> -
> -ifeq ($(strip $(MESA_BUILD_GALLIUM)),true)
> -LOCAL_REQUIRED_MODULES += gallium_dri
> -endif # MESA_BUILD_GALLIUM
> -
> +# This controls enabling building of driver libraries
> +LOCAL_REQUIRED_MODULES += $(if $(HAVE_I915_DRI),i915_dri,)
> +LOCAL_REQUIRED_MODULES += $(if $(HAVE_I965_DRI),i965_dri,)
> +LOCAL_REQUIRED_MODULES += $(if $(MESA_BUILD_GALLIUM),gallium_dri,)
>
Can I interest you in expanding the ifs as originally? Having control
flow on a single line can be a bit confusing.

>  LOCAL_MODULE := libGLES_mesa
>  LOCAL_MODULE_RELATIVE_PATH := egl
> 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.

> 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?

> +ifneq ($(HAVE_GALLIUM_R600),)
> +$(eval GALLIUM_LIBS += $(LOCAL_MODULE) $(LOCAL_STATIC_LIBRARIES) \
> +       libmesa_winsys_radeon)
Nit: maybe write this as below. We're a bit inconsistent in mesa so
feel free to ignore.

$(eval GALLIUM_LIBS += \
       $(LOCAL_MODULE) \
       $(LOCAL_STATIC_LIBRARIES) \
       libmesa_winsys_radeon)



>  LOCAL_WHOLE_STATIC_LIBRARIES := \
> -       $(gallium_DRIVERS) \
> +       $(sort $(GALLIUM_LIBS)) \
Please add a small comment about sort.
We use sort with GALLIUM_LIBS and GALLIUM_SHARED_LIBS in order to
remove any duplicate libraries.


> --- a/src/gallium/winsys/amdgpu/drm/Android.mk
> +++ b/src/gallium/winsys/amdgpu/drm/Android.mk
> @@ -41,3 +41,8 @@ LOCAL_MODULE := libmesa_winsys_amdgpu
>
>  include $(GALLIUM_COMMON_MK)
>  include $(BUILD_STATIC_LIBRARY)
> +
> +ifneq ($(HAVE_GALLIUM_RADEONSI),)
> +$(eval GALLIUM_LIBS += $(LOCAL_MODULE) $(LOCAL_STATIC_LIBRARIES))
> +$(eval GALLIUM_SHARED_LIBS += $(LOCAL_SHARED_LIBRARIES))
> +endif
> 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.


Thanks
Emil