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

[Emil Velikov]

On 22 February 2018 at 13:23, Thierry Reding <thierry.reding at 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.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.
>
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