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

[Christian Gmeiner]

Hi Emil,

2015-10-16 1:33 GMT+02:00 Emil Velikov <emil.l.velikov at 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.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.
>

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