On 22 February 2018 at 13:23, Thierry Reding <thierry.red...@gmail.com> wrote: > 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.red...@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. > Great - one less thing to think about ;-)
>> >> 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. > Don't worry, I'll address those. With a comment what to tweak if libdrm_tegra becomes a dependency. > > 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. > Ack. Since this can be tweaked later, I'd suggest not blocking the series on the name specifics. >> > + >> > + 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. > My train of thought is as follows, although it might be tad pedantic. Like in the kernel HW enablement patches are allowed in -stable. Yet trying to retroactively fix a glitch is more fiddly. Regardless, it's just an idea. >> > +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. > As you can see the winsys split isn't as distinct on some drivers, so trying to cross check is fiddly. Regardless which option you go for, a small TODO/NOTE might be a good idea. HTH Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev