Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190123000057.31477-1-oded.gabbay@gmail.com> <CAPM=9tznaiw+a7rWqfdMMO6Aj3GtsO9vetd=3n9d+bGLW0U8Cg@mail.gmail.com>
 <20190125073745.GD11891@kroah.com>
In-Reply-To: <20190125073745.GD11891@kroah.com>
From:   Olof Johansson <olof@lixom.net>
Date:   Fri, 25 Jan 2019 07:33:23 -0800
Message-ID: <CAOesGMgJUeUMoRXZ98=J8bpfCuZOOMo5pwpVXZX-o-y1dAQsHA@mail.gmail.com>
Subject: Re: [PATCH 00/15] Habana Labs kernel driver
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Dave Airlie <airlied@gmail.com>,
        Oded Gabbay <oded.gabbay@gmail.com>,
        Jerome Glisse <jglisse@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>, ogabbay@habana.ai
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Jan 24, 2019 at 11:37 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 24, 2019 at 07:57:11AM +1000, Dave Airlie wrote:
> > On Wed, 23 Jan 2019 at 10:01, Oded Gabbay <oded.gabbay@gmail.com> wrote:
> > >
> > > Hello,
> > >
> > > For those who don't know me, my name is Oded Gabbay (Kernel Maintainer
> > > for AMD's amdkfd driver, worked at RedHat's Desktop group) and I work at
> > > Habana Labs since its inception two and a half years ago.
> >
> > Hey Oded,
> >
> > So this creates a driver with a userspace facing API via ioctls.
> > Although this isn't a "GPU" driver we have a rule in the graphics
> > drivers are for accelerators that we don't merge userspace API with an
> > appropriate userspace user.
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> >
> > I see nothing in these accelerator drivers that make me think we
> > should be treating them different.
>
> I understand that this is your position on when you accept drivers into
> the DRM layer, as you need to interact with common interfaces and a
> massive userspace stack at the same time.  And that's wonderful, it
> allows you to be able to move both sides of that stack forward without
> removing support for devices that worked on older kernels.
>
> But, that's not really the case with this new driver at all.  We add new
> driver subsystems, and individual drivers, with loads of new ioctls, in
> every new kernel release.  We don't impose on all of them the "your
> userspace code must be open" rule, so why is this new driver somehow
> different from them?
>
> Yes, there is the fun legal issue of "derivative works" when talking
> about a userspace program that is written to only interact with a
> specific kernel driver using a custom api like this one has, and how the
> license of the kernel side (GPLv2) affects the userspace side
> (whatever), but that is something that I leave up to the lawyers who
> like discussing and enforcing such things.
>
> When evaluating this driver (note, I saw it for a few revisions before
> Oded posted it here), all I did was try to make sure that it fit in
> properly with the kernel apis and methods of operations.  Given that
> there are no in-kernel drivers for this type of device, and that it
> really is a pretty small shim layer around the hardware, which means
> that userspace does a lot of the heavy lifting, it is going to be a
> very hardware-specific user/kernel api, and that shows.

I brought this up because there are sort of, if you squint, three-ish
of these already (the OpenCAPI/CAPI ones and now this). Also,
Jonathan's comment about CCIX pieces coming as well.

I've talked to a handful of vendors in this space and more or less all
of them hope that drivers/misc is still a suitable home for their
driver by the time it's ready to post, and I know that if we keep them
there, it'll only be one or two more drivers until we have this
discussion anyway.

That's really why I brought it up now, to get a clear stance on "Yeah,
we know these are all slightly different today, and we're willing to
give them a predictable home for the first while as we figure out
together how things should look". Keep in mind, this space is also
currently a bit of a gold rush, with many people working hard to get
to market with near-zero talk between them.

> Sidenote, this could have almost just been a UIO driver, which would
> have put _ALL_ of the logic in userspace.  At least this way we have a
> chance for the kernel code to be sane and not try to inflict problems on
> the rest of the system.

Yeah, and sharing hardware when you have userspace drivers tends to
need a bunch of synchronization and coordination which leads to taller
(closed?) stacks there. Having resource arbitration and sharing
assisted by the kernel is usually the right thing, and we should
encourage that.

> Going forward, it would be wonderful if we could come up with a unified
> api for how to interact with these types of hardware accelerators, but
> given the newness of this industry, and the vastly different ways people
> are trying to solve the problem, that is going to take a few attempts,
> and many years before we can get there.  Until then, taking drivers like
> this into the kernel tree makes sense as that way all of our users will
> be able to use that hardware, and better yet, the rest of us can learn
> more about how this stuff works so that we can help out with that api
> generation when it happens.

Exactly my viewpoint as well, combined with not pushing vendors
towards nvidia models by default even if they start out separate, and
having a chance to find inroads with them on engineering levels when
they come to us with the drivers.

Absolutely best chance for success is _always_ when we can engage with
the vendor engineers as peers, instead of going up and down the
corporate charts first (you often need to do a bit of both, but having
allies in engineering makes it much easier).

> So for now, I have no objection to taking this type of driver into the
> tree.  Yes, it would be wonderful if we had an open userspace program to
> drive it so that we could actually test and make changes to the api over
> time, but I think that is something that the submitting company needs to
> realize will be better for them to do, as for right now, all of that
> testing and changes are their responsibility.

I do think having requirements of being able to exercise the hardware
is really valuable, and we should consider what a requirement for that
would look like. For the Habana case that could be a separate
low-level library and a test workload on top. For FPGA cases it could
be a well-known reference bitstream with the vendor reference
control/communication path + similar low-level pieces.

> As for what directory the code should live in, I suggested "misc" as
> there was no other universal location, and I hate to see new subsystems
> be created with only one driver, as that's pretty sad.  But it's just a
> name/location, I have no dog in the fight, so I really don't care where
> it ends up in the tree, just as long as it gets merged somewhere :)

I'm usually one to push back against new subsystems too, especially
when I see a framework proposal with just one driver. In this case,
given that we all know more vendors will come along, I think it makes
sense to take the discussion and establish structure now. This should
give some clarity to those who are out there that we haven't seen yet,
and give them a chance to prepare for things such as the low-level
userspace pieces mentioned above.

So I think setting this up now is the right thing to do, we know there
will be more material here and having a common aggregation of it makes
sense.


-Olof