Hi Emil, 2015-10-16 1:33 GMT+02:00 Emil Velikov <emil.l.veli...@gmail.com>: > Hi Christian, > > Mostly minor suggestions I'm afraid. Things just look too good for > anything serious. >
:) > On 11 October 2015 at 16:09, Christian Gmeiner > <christian.gmei...@gmail.com> wrote: >> This commit adds tegra support, which uses the renderonly driver >> library. >> >> Signed-off-by: Christian Gmeiner <christian.gmei...@gmail.com> >> --- >> configure.ac | 19 +++++++- >> src/gallium/Makefile.am | 6 +++ >> .../auxiliary/target-helpers/inline_drm_helper.h | 29 ++++++++++++ >> src/gallium/drivers/tegra/Automake.inc | 10 +++++ >> src/gallium/drivers/tegra/Makefile.am | 9 ++++ >> src/gallium/targets/dri/Makefile.am | 2 + >> src/gallium/winsys/tegra/drm/Android.mk | 34 +++++++++++++++ >> src/gallium/winsys/tegra/drm/Makefile.am | 33 ++++++++++++++ >> src/gallium/winsys/tegra/drm/Makefile.sources | 3 ++ >> src/gallium/winsys/tegra/drm/tegra_drm_public.h | 31 +++++++++++++ >> src/gallium/winsys/tegra/drm/tegra_drm_winsys.c | 51 >> ++++++++++++++++++++++ >> 11 files changed, 226 insertions(+), 1 deletion(-) >> create mode 100644 src/gallium/drivers/tegra/Automake.inc >> create mode 100644 src/gallium/drivers/tegra/Makefile.am >> create mode 100644 src/gallium/winsys/tegra/drm/Android.mk >> create mode 100644 src/gallium/winsys/tegra/drm/Makefile.am >> create mode 100644 src/gallium/winsys/tegra/drm/Makefile.sources >> create mode 100644 src/gallium/winsys/tegra/drm/tegra_drm_public.h >> create mode 100644 src/gallium/winsys/tegra/drm/tegra_drm_winsys.c >> >> diff --git a/configure.ac b/configure.ac >> index ea485b1..9fb8244 100644 >> --- a/configure.ac >> +++ b/configure.ac > [snip] >> @@ -2166,6 +2167,12 @@ if test -n "$with_gallium_drivers"; then >> HAVE_GALLIUM_LLVMPIPE=yes >> fi >> ;; >> + xtegra) >> + HAVE_GALLIUM_TEGRA=yes > We need an extra NEED_GALLIUM_NOUVEAU conditional (set to yes here and > in the xnouveau case). > One will also need to duplicate (as a temporary workaround) the > nouveau PKG_CHECK_MODULES here. > > Then update the src/gallium/Makefile.am to use it over HAVE_GALLIUM_NOUVEAU > > [snip] >> +dnl We need to validate some needed dependencies for renderonly drivers. >> + >> +if test "x$HAVE_GALLIUM_NOUVEAU" != xyes -a "x$HAVE_GALLIUM_TEGRA" == xyes >> ; then >> + AC_ERROR([Building with tegra requires that nouveau]) >> +fi >> + >> + > And then you can drop this hunk. > I will give it a try. >> --- a/src/gallium/auxiliary/target-helpers/inline_drm_helper.h >> +++ b/src/gallium/auxiliary/target-helpers/inline_drm_helper.h >> @@ -59,6 +59,10 @@ >> #include "vc4/drm/vc4_drm_public.h" >> #endif >> >> +#if GALLIUM_TEGRA >> +#include "tegra/drm/tegra_drm_public.h" >> +#endif >> + > FYI, I'm just testing some updates/rewrites of these target-helpers, > so things might clash in the not so distant future. > Yeah I have seen your patch set and I am prepared to rework some parts. > [snip] >> --- /dev/null >> +++ b/src/gallium/drivers/tegra/Automake.inc >> @@ -0,0 +1,10 @@ >> +if HAVE_GALLIUM_TEGRA >> + >> +TARGET_DRIVERS += tegra >> +TARGET_CPPFLAGS += -DGALLIUM_TEGRA >> +TARGET_LIB_DEPS += \ >> + $(top_builddir)/src/gallium/drivers/renderonly/librenderonly.la \ >> + $(top_builddir)/src/gallium/winsys/tegra/drm/libtegradrm.la \ >> + $(LIBDRM_LIBS) > This, perhaps, should be TEGRA_LIBS, yet we're not using anything from > libdrm_tegra so we should be safe. > I think I have tried that but linking fails due duplicate symbols. > [snip] >> --- /dev/null >> +++ b/src/gallium/winsys/tegra/drm/Android.mk > I think we can drop this file for now. Android + tegra is quite > incomplete as is. > Great. > [snip] >> --- /dev/null >> +++ b/src/gallium/winsys/tegra/drm/tegra_drm_winsys.c > [snip] >> +#include "renderonly/renderonly_screen.h" >> +#include "../winsys/tegra/drm/tegra_drm_public.h" >> +#include "../winsys/nouveau/drm/nouveau_drm_public.h" >> + > Please rework things to avoid the ../'s > Okay. >> +#include <drm/tegra_drm.h> > (as mentioned before) Please drop the path from the include. > >> +#include <xf86drm.h> >> + > Flip these two and move them to the top ? > Sure. >> +static int tegra_tiling(int fd, uint32_t handle) >> +{ >> + struct drm_tegra_gem_set_tiling args; >> + >> + memset(&args, 0, sizeof(args)); >> + args.handle = handle; >> + args.mode = DRM_TEGRA_GEM_TILING_MODE_BLOCK; >> + args.value = 4; > Worth adding a note wrt the magic number ? > Yes, magic numbers are bad - will fix it. greets -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev