[Mesa-dev] [PATCH 5/5] tegra: Initial support

[Thierry Reding]

On Wed, Feb 21, 2018 at 05:01:02PM +0000, Emil Velikov wrote:
> Hi Thierry,
> 
> On 21 February 2018 at 15:30, Thierry Reding <thierry.reding at gmail.com> wrote:
> 
> > @@ -2595,6 +2596,11 @@ if test -n "$with_gallium_drivers"; then
> >         ximx)
> >              HAVE_GALLIUM_IMX=yes
> >              ;;
> > +        xtegra)
> > +            HAVE_GALLIUM_TEGRA=yes
> > +            PKG_CHECK_MODULES([TEGRA], [libdrm_tegra >= $LIBDRM_TEGRA_REQUIRED])
> > +            require_libdrm "tegra"
> > +            ;;
> 
> Could use the following hunk just after the "... needed dependencies
> for renderonly drivers." comment.
> 
> if test "x$HAVE_GALLIUM_NOUVEAU" != xyes -a "x$HAVE_GALLIUM_TEGRA" =
> xyes  ; then
>     AC_MSG_ERROR([Building with tegra requires nouveau])
> fi

Done. It also turns out that we don't need libdrm_tegra (yet), so I can
drop that, which makes it easier to build-test.

> 
> In a future patch we can:
> - add tegra to the Makefile.am DISTCHECK list

I can do that. With the libdrm_tegra dependency gone that should be no
problem. But what if we want to add it back at some point? Are people
supposed to have all the dependencies installed for a make distcheck?

> - wire up the driver with .travis

I'm not very familiar with Travis CI on Mesa. Is there a public instance
that I can check? Also, is there a way to test such a change before
pushing it to make sure we don't inadvertently break it?

Well, I guess that's kind of the point of Travis, but I means things
like making sure the .travis syntax and build dependencies are correct,
and so on.

> > index 000000000000..910cbe02d873
> > --- /dev/null
> > +++ b/include/drm-uapi/tegra_drm.h
> 
> Haven't checked this one, but I trust you picked the latest and greatest.

Yeah, this is from v4.16-rc1, though I've updated the include guards to
drop the UAPI from Linux' headers. That seemed to be what others were
doing, so hopefully that was correct.

> > --- /dev/null
> > +++ b/src/gallium/drivers/tegra/Makefile.am
> > @@ -0,0 +1,18 @@
> > +AUTOMAKE_OPTIONS = subdir-objects
> > +
> Not needed - already set at global scope.

Done.

> 
> > +include Makefile.sources
> > +include $(top_srcdir)/src/gallium/Automake.inc
> > +
> > +AM_CFLAGS = \
> > +       -I$(top_srcdir)/include/drm-uapi \
> > +       $(GALLIUM_DRIVER_CFLAGS) \
> > +       $(LIBUDEV_CFLAGS) \
> libudev is gone.

Done. I also removed the TEGRA_CFLAGS which is not (yet) needed.

> 
> > +
> > +noinst_LTLIBRARIES = libtegra.la
> > +
> > +libtegra_la_SOURCES = \
> > +       $(C_SOURCES)
> > +
> > +libtegra_la_LIBADD = \
> > +       $(LIBUDEV_LIBS)
> Ditto.

Done.

> 
> 
> > --- /dev/null
> > +++ b/src/gallium/drivers/tegra/Makefile.sources
> > @@ -0,0 +1,3 @@
> > +C_SOURCES := \
> > +       tegra_context.c \
> > +       tegra_screen.c
> 
> Please include the following to as well.
> tegra_context.h
> tegra_resource.h

Done.

> > index 000000000000..feaa5138c95d
> > --- /dev/null
> > +++ b/src/gallium/drivers/tegra/tegra_context.c
> 
> > +#include "tegra/tegra_context.h"
> > +#include "tegra/tegra_resource.h"
> > +#include "tegra/tegra_screen.h"
> > +
> The "tegra/" prefix isn't needed - it doesn't hurt though.

Removed.

> > --- /dev/null
> > +++ b/src/gallium/drivers/tegra/tegra_screen.c
> 
> > +#include "tegra/tegra_context.h"
> > +#include "tegra/tegra_resource.h"
> > +#include "tegra/tegra_screen.h"
> > +
> Ditto.

Removed.

> > +static const char *
> > +tegra_screen_get_name(struct pipe_screen *pscreen)
> > +{
> > +   return "tegra";
> > +}
> > +
> > +static const char *
> > +tegra_screen_get_vendor(struct pipe_screen *pscreen)
> > +{
> > +   return "NVIDIA";
> > +}
> > +
> > +static const char *
> > +tegra_screen_get_device_vendor(struct pipe_screen *pscreen)
> > +{
> > +   return "NVIDIA";
> > +}
> > +
> As-is these might be a bit fiddly, but will do for now.
> For example - how will devs distinguish between the closed-source
> driver and Mesa.

Good point. Let me check what exactly we use in the closed-source driver
and then come up with a proposal.

I think perhaps a good choice for the vendor would be "grate". Even
though this driver isn't part of the grate project, I'm hoping that we
will eventually see Erik's and Dmitry's work merged into this.

Adding Erik and Dmitry to get their opinion.

> > +static int tegra_open_render_node(void)
> > +{
> > +   drmDevicePtr *devices, device;
> > +   int err, fd = -ENODEV;
> > +   unsigned int num, i;
> > +
> > +   err = drmGetDevices2(0, NULL, 0);
> > +   if (err < 0)
> > +      return err;
> > +
> > +   num = err;
> > +
> > +   devices = calloc(num, sizeof(*devices));
> > +   if (!devices)
> > +      return -ENOMEM;
> > +
> > +   err = drmGetDevices2(0, devices, num);
> > +   if (err < 0) {
> > +      free(devices);
> > +      return err;
> > +   }
> > +
> > +   for (i = 0; i < num; i++) {
> > +      device = devices[i];
> > +
> > +      if ((device->available_nodes & (1 << DRM_NODE_RENDER)) &&
> > +          (device->bustype == DRM_BUS_PLATFORM)) {
> Worth checking for any instances of "tegra" in the platform/deviceinfo strings?

This is actually checking for the Tegra GPU driven by Nouveau. The
"tegra" device would be on DRM_BUS_HOST1X. And we kind of know that
there will always only be one GPU on the platform bus.

I could add a check for the driver, just not sure it's worth it.

> 
> > +         fd = open(device->nodes[DRM_NODE_RENDER], O_RDWR);
> open(.... O_RDWR | O_CLOEXEC) + check for failure.

Done.

> 
> 
> 
> > +struct pipe_screen *
> > +tegra_screen_create(int fd)
> > +{
> > +   struct tegra_screen *screen;
> > +
> > +   screen = calloc(1, sizeof(*screen));
> > +   if (!screen)
> > +      return NULL;
> > +
> > +   screen->fd = fd;
> IIRC there were some fallouts for xinerama setups that required a dup().
> Don't recall the details and how applicable those are here.

Others seem to be doing this as well, though usually via the winsys
rather than the driver. Looks like etnaviv does a regular dup() but
vc4 does it via fcntl() to add O_CLOEXEC. The latter seems safer to
me. I'll go with that.

Thanks again for the review!

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180222/f13df465/attachment.sig>