Hi Emil, thanks for your review!
2017-01-09 21:00 GMT+01:00 Emil Velikov <emil.l.veli...@gmail.com>: > 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.gmei...@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. > Added something :) > >> --- /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 ?) > Fixed in separate patch which gets squashed before pushing. >> +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 > Fixed in separate patch which gets squashed before pushing. > >> --- /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 ? > Fixed in separate patch which gets squashed before pushing. > >> --- /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 ? > Fixed in separate patch which gets squashed before pushing. greets -- Christian Gmeiner, MSc https://www.youtube.com/user/AloryOFFICIAL https://soundcloud.com/christian-gmeiner _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev