[Mesa-dev] [PATCH v2 2/3] etnaviv: gallium driver for Vivante GPUs

[Emil Velikov]

Hi Christian,

There's a few nitpicks. Note that neither is a blocker so feel free to
send patches in the week(s) to come.

On 23 December 2016 at 22:04, Christian Gmeiner
<christian.gmeiner at gmail.com> wrote:

> --- a/configure.ac
> +++ b/configure.ac
> @@ -76,6 +76,7 @@ LIBDRM_NVVIEUX_REQUIRED=2.4.66
>  LIBDRM_NOUVEAU_REQUIRED=2.4.66
>  LIBDRM_FREEDRENO_REQUIRED=2.4.74
>  LIBDRM_VC4_REQUIRED=2.4.69
> +LIBDRM_ETNAVIV_REQUIRED=2.4.74
>  DRI2PROTO_REQUIRED=2.6
>  DRI3PROTO_REQUIRED=1.0
>  PRESENTPROTO_REQUIRED=1.0
> @@ -2506,6 +2507,11 @@ if test -n "$with_gallium_drivers"; then
>              PKG_CHECK_MODULES([FREEDRENO], [libdrm_freedreno >= $LIBDRM_FREEDRENO_REQUIRED])
>              require_libdrm "freedreno"
>              ;;
> +        xetnaviv)
> +            HAVE_GALLIUM_ETNAVIV=yes
> +            PKG_CHECK_MODULES([ETNAVIV], [libdrm_etnaviv >= $LIBDRM_ETNAVIV_REQUIRED])
> +            require_libdrm "etnaviv"
> +            ;;
- There's a comment/documentation missing why we want/need
etnaviv_dri.so anywhere.
Please add something - be that in configure or a readme file in
{drivers,winsys}/etnaviv.

Pretty much anything will do ;-)

- This and 3/3 will need to update the "AC_ARG_WITH([gallium-drivers]"
help string.
Again - perfectly fine to address with separate commit.


> --- /dev/null
> +++ b/src/gallium/drivers/etnaviv/Makefile.am

> +
> +libetnaviv_la_SOURCES = $(C_SOURCES) $(CPP_SOURCES)
> +
CPP_SOURCES is empty. Please remove it (with later commit ?)

> +noinst_PROGRAMS = etnaviv_compiler
> +
> +etnaviv_compiler_SOURCES = \
> +       etnaviv_compiler_cmdline.c
> +
> +etnaviv_compiler_LDADD = \
> +       libetnaviv.la \
> +       ../../auxiliary/libgallium.la \
Don't use relative paths, use the below instead.

$(top_builddir)/src/gallium/auxiliary/libgallium.la


> --- /dev/null
> +++ b/src/gallium/drivers/etnaviv/Makefile.sources
> @@ -0,0 +1,26 @@
> +C_SOURCES :=  \
> +       etnaviv_asm.c \
> +       etnaviv_blend.c \
> +       etnaviv_clear_blit.c \
> +       etnaviv_compiler.c \
> +       etnaviv_context.c \
> +       etnaviv_disasm.c \
> +       etnaviv_emit.c \
> +       etnaviv_fence.c \
> +       etnaviv_format.c \
> +       etnaviv_query.c \
> +       etnaviv_query_sw.c \
> +       etnaviv_rasterizer.c \
> +       etnaviv_resource.c \
> +       etnaviv_rs.c \
> +       etnaviv_screen.c \
> +       etnaviv_shader.c \
> +       etnaviv_state.c \
> +       etnaviv_surface.c \
> +       etnaviv_tiling.c \
> +       etnaviv_texture.c \
> +       etnaviv_transfer.c \
> +       etnaviv_uniforms.c \
> +       etnaviv_zsa.c
> +
Iirc earlier commits had all the files (headers including) listed here.
Can we have those back please ?


> --- /dev/null
> +++ b/src/gallium/winsys/etnaviv/drm/etnaviv_drm_winsys.c

> +static struct pipe_screen *
> +screen_create(struct renderonly *ro)
> +{
> +   struct etna_device *dev;
> +   struct etna_gpu *gpu;
> +   uint64_t val;
> +   int i;
> +
> +   dev = etna_device_new_dup(ro->gpu_fd);
> +   if (!dev) {
> +      fprintf(stderr, "Error creating device\n");
> +      return NULL;
> +   }
> +
> +   for (i = 0;; i++) {
> +      gpu = etna_gpu_new(dev, i);
> +      if (!gpu) {
> +         fprintf(stderr, "Error creating gpu\n");
> +         return NULL;
> +      }
> +
> +      /* Look for a 3D capable GPU */
> +      if (etna_gpu_get_param(gpu, ETNA_GPU_FEATURES_0, &val) == 0 &&
> +            val & (1 << 2))
We cannot used the VIV_FEATURE macro yet, but can we replace "1 << 2"
with the corresponding define ?

Thanks
Emil