[Mesa-dev] [RFC 2/2] gallium: add tegra support

[Emil Velikov]

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.gmeiner at gmail.com> wrote:
> This commit adds tegra support, which uses the renderonly driver
> library.
>
> Signed-off-by: Christian Gmeiner <christian.gmeiner at 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.

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

[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.

[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.

[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

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

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

Thanks
Emil