[Mesa-dev] [PATCH v1 6/7] android: add etnaviv driver build support

Fri Jun 16 13:24:41 UTC 2017

Hi Rob,

There's a handful of small nitpicks, but nothing serious.

On 15 June 2017 at 21:47, Robert Foss <robert.foss at collabora.com> wrote:
> From: Rob Herring <robh at kernel.org>
>
> Add etnaviv to Android makefiles.
>
> Signed-off-by: Rob Herring <robh at kernel.org>
> ---
>  Android.mk                                    |  5 +--
>  src/gallium/Android.mk                        |  1 +
>  src/gallium/auxiliary/Android.mk              |  5 +--
>  src/gallium/auxiliary/renderonly/renderonly.c |  1 +
>  src/gallium/drivers/etnaviv/Android.mk        | 47 +++++++++++++++++++++++++++
>  src/gallium/winsys/etnaviv/drm/Android.mk     | 35 ++++++++++++++++++++
>  6 files changed, 90 insertions(+), 4 deletions(-)
>  create mode 100644 src/gallium/drivers/etnaviv/Android.mk
>  create mode 100644 src/gallium/winsys/etnaviv/drm/Android.mk
>
> diff --git a/Android.mk b/Android.mk
> index de37f4600f..b4dc8321a6 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -24,7 +24,7 @@
>  # BOARD_GPU_DRIVERS should be defined.  The valid values are
>  #
>  #   classic drivers: i915 i965
> -#   gallium drivers: swrast freedreno i915g nouveau r300g r600g radeonsi vc4 virgl vmwgfx
> +#   gallium drivers: swrast freedreno i915g nouveau r300g r600g radeonsi vc4 virgl vmwgfx etnaviv
>  #
>  # The main target is libGLES_mesa.  For each classic driver enabled, a DRI
>  # module will also be built.  DRI modules will be loaded by libGLES_mesa.
> @@ -56,7 +56,8 @@ gallium_drivers := \
>         radeonsi.HAVE_GALLIUM_RADEONSI \
>         vmwgfx.HAVE_GALLIUM_VMWGFX \
>         vc4.HAVE_GALLIUM_VC4 \
> -       virgl.HAVE_GALLIUM_VIRGL
> +       virgl.HAVE_GALLIUM_VIRGL \
> +       etnaviv.HAVE_GALLIUM_ETNAVIV
>
>  ifeq ($(BOARD_GPU_DRIVERS),all)
>  MESA_BUILD_CLASSIC := $(filter HAVE_%, $(subst ., , $(classic_drivers)))
> diff --git a/src/gallium/Android.mk b/src/gallium/Android.mk
> index 0915579127..0280574405 100644
> --- a/src/gallium/Android.mk
> +++ b/src/gallium/Android.mk
> @@ -43,6 +43,7 @@ SUBDIRS += winsys/radeon/drm winsys/amdgpu/drm drivers/radeonsi drivers/radeon
>  SUBDIRS += winsys/vc4/drm drivers/vc4
>  SUBDIRS += winsys/virgl/drm winsys/virgl/vtest drivers/virgl
>  SUBDIRS += winsys/svga/drm drivers/svga
> +SUBDIRS += winsys/etnaviv/drm drivers/etnaviv drivers/renderonly
>  SUBDIRS += state_trackers/dri
>
>  # sort to eliminate any duplicates
> diff --git a/src/gallium/auxiliary/Android.mk b/src/gallium/auxiliary/Android.mk
> index e2a1fc214e..a562774a9a 100644
> --- a/src/gallium/auxiliary/Android.mk
> +++ b/src/gallium/auxiliary/Android.mk
> @@ -31,7 +31,8 @@ include $(CLEAR_VARS)
>  LOCAL_SRC_FILES := \
>         $(C_SOURCES) \
>         $(NIR_SOURCES) \
> -       $(VL_STUB_SOURCES)
> +       $(VL_STUB_SOURCES) \
> +       $(RENDERONLY_SOURCES)
>
>  LOCAL_C_INCLUDES := \
>         $(GALLIUM_TOP)/auxiliary/util
> @@ -46,7 +47,7 @@ LOCAL_CPPFLAGS += -std=c++11
>
>  # We need libmesa_nir to get NIR's generated include directories.
>  LOCAL_MODULE := libmesa_gallium
> -LOCAL_STATIC_LIBRARIES += libmesa_nir
> +LOCAL_STATIC_LIBRARIES += libmesa_nir libmesa_loader
>
Do we really need the loader here? Afaict module does not export any
headers/etc that we need?
We should only need it in the final link stage, although it's there already.

>  LOCAL_WHOLE_STATIC_LIBRARIES += cpufeatures
>
> diff --git a/src/gallium/auxiliary/renderonly/renderonly.c b/src/gallium/auxiliary/renderonly/renderonly.c
> index d3ed214f4e..dda7f4471a 100644
> --- a/src/gallium/auxiliary/renderonly/renderonly.c
> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
> @@ -35,6 +35,7 @@
>  #include "pipe/p_screen.h"
>  #include "util/u_inlines.h"
>  #include "util/u_memory.h"
> +#include "loader.h"
>
Seemingly unrelated change. Please split out if we really need it.

>  struct renderonly *
>  renderonly_dup(const struct renderonly *ro)
> diff --git a/src/gallium/drivers/etnaviv/Android.mk b/src/gallium/drivers/etnaviv/Android.mk
> new file mode 100644
> index 0000000000..7e9b2ae1b2
> --- /dev/null
> +++ b/src/gallium/drivers/etnaviv/Android.mk
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2016 Linaro, Ltd, Rob Herring <robh at kernel.org>
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice shall be included
> +# in all copies or substantial portions of the Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +# DEALINGS IN THE SOFTWARE.
> +
> +LOCAL_PATH := $(call my-dir)
> +
> +# get C_SOURCES
> +include $(LOCAL_PATH)/Makefile.sources
> +
> +include $(CLEAR_VARS)
> +
> +LOCAL_SRC_FILES := \
> +       $(C_SOURCES)
> +
> +LOCAL_C_INCLUDES := \
> +       $(LOCAL_PATH)/hw
> +
Cannot see this include in the Automake build so, it doesn't seem like
we need it?

> +#LOCAL_GENERATED_SOURCES := $(MESA_GEN_NIR_H)
> +
> +LOCAL_SHARED_LIBRARIES := libdrm_etnaviv
> +#LOCAL_STATIC_LIBRARIES := libmesa_glsl libmesa_nir
> +LOCAL_MODULE := libmesa_pipe_etnaviv
> +
> +#include $(LOCAL_PATH)/Android.gen.mk
Please drop the commented out lines.

> --- /dev/null
> +++ b/src/gallium/winsys/etnaviv/drm/Android.mk

> +# get C_SOURCES
> +#include $(LOCAL_PATH)/Makefile.sources
Uncomment this line...

> +
> +include $(CLEAR_VARS)
> +
> +LOCAL_SRC_FILES := etnaviv_drm_winsys.c
... and use the sources variable $(C_SOURCES)

-Emil