Date: Wed, 26 Jun 2024 09:26:40 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Oded Gabbay <ogabbay@...nel.org>
Cc: Tomeu Vizoso <tomeu@...euvizoso.net>,
	Jeffrey Hugo <quic_jhugo@...cinc.com>, linux-kernel@...r.kernel.org,
	Lucas Stach <l.stach@...gutronix.de>,
	Russell King <linux+etnaviv@...linux.org.uk>,
	Christian Gmeiner <christian.gmeiner@...il.com>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	etnaviv@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only

On Thu, May 09, 2024 at 05:41:18PM +0300, Oded Gabbay wrote:
> On Thu, May 09, 2024 at 03:53:01PM +0200, Tomeu Vizoso wrote:
> > Oded, Dave,
> > 
> > Do you have an opinion on this?
> > 
> > Thanks,
> > 
> > Tomeu
> Hi Tomeu,
> 
> Sorry for not replying earlier, I was down with Covid (again...).
> 
> To your question, I don't have an objection to what you are
> suggesting. My personal view of accel is that it is an integral part of 
> DRM and therefore, if there is an *existing* drm driver that wants to 
> create an accel node, I'm not against it. 

Yeah, there's a continum from "clearly 3d gpu" to "compute AI
accelerator", with everything possible in-between shipping somewhere.
Collaboration is the important part, hair-splitting on where exactly the
driver should be is kinda secondary. I mean beyond "don't put a pure 3d
driver into accel or vice versa" of course :-)

> There is the question of why you want to expose an accel node, and
> here I would like to hear Dave's and Sima's opinion on your suggested
> solution as it may affect the direction of other drm drivers.

So existing userspace that blindly assumes that any render node will give
it useful 3d acceleration, then that's broken already.

- kernel with new driver support but old mesa without that driver already
  gives you that, even for a pure 3d chip.

- intel (and I think also amd) have pure compute chips without 3d, so this
  issue already exists

Same for the other directions, 3d gpus have variable amounts of compute
chips nowadays.

That leaves imo just the pragmatic choice, and if we need to complicate
the init flow of the kernel driver just for a different charnode major,
then I don't really see the point.

And if we do see the point in this, I think the right approach would be if
we split the init flow further into allocating the drm_device, and then in
a 2nd step either allocate the accel or render uapi stuff as needed. The
DRIVER_FOO flags just aren't super flexible for this kinda of stuff and
have a bit a midlayer taste to them.

Cheers, Sima

> 
> Thanks,
> Oded.
> 
> p.s.
> Please only use bottom-posting when replying, thanks :)
> 
> > 
> > On Fri, Apr 26, 2024 at 8:10 AM Tomeu Vizoso <tomeu@...euvizoso.net> wrote:
> > >
> > > On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo <quic_jhugo@...cinc.com> wrote:
> > > >
> > > > On 4/24/2024 12:37 AM, Tomeu Vizoso wrote:
> > > > > If we expose a render node for NPUs without rendering capabilities, the
> > > > > userspace stack will offer it to compositors and applications for
> > > > > rendering, which of course won't work.
> > > > >
> > > > > Userspace is probably right in not questioning whether a render node
> > > > > might not be capable of supporting rendering, so change it in the kernel
> > > > > instead by exposing a /dev/accel node.
> > > > >
> > > > > Before we bring the device up we don't know whether it is capable of
> > > > > rendering or not (depends on the features of its blocks), so first try
> > > > > to probe a rendering node, and if we find out that there is no rendering
> > > > > hardware, abort and retry with an accel node.
> > > > >
> > > > > Signed-off-by: Tomeu Vizoso <tomeu@...euvizoso.net>
> > > > > Cc: Oded Gabbay <ogabbay@...nel.org>
> > > >
> > > > I hope Oded chimes in as Accel maintainer.  I think Airlie/Vetter had
> > > > also previously mentioned they'd have opinions on what is Accel vs DRM.
> > > >
> > > > This gets a nack from me in its current state.  This is not a strong
> > > > nack, and I don't want to discourage you.  I think there is a path forward.
> > > >
> > > > The Accel subsystem documentation says that accel drivers will reside in
> > > > drivers/accel/ but this does not.
> > >
> > > Indeed, there is that code organization aspect.
> > >
> > > > Also, the commit text for "accel: add dedicated minor for accelerator
> > > > devices" mentions -
> > > >
> > > > "for drivers that
> > > > declare they handle compute accelerator, using a new driver feature
> > > > flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
> > > > driver feature is mutually exclusive with DRIVER_RENDER. Devices that
> > > > want to expose both graphics and compute device char files should be
> > > > handled by two drivers that are connected using the auxiliary bus
> > > > framework."
> > > >
> > > > I don't see any of that happening here (two drivers connected by aux
> > > > bus, one in drivers/accel).
> > >
> > > Well, the text refers to devices, not drivers. The case we are talking
> > > about is a driver that wants to sometimes expose an accel node, and
> > > sometimes a render node, depending on the hardware it is dealing with.
> > > So there would either be a device exposing a single render node, or a
> > > device exposing a single accel node.
> > >
> > > Though by using the auxiliary bus we could in theory solve the code
> > > organization problem mentioned above, I'm not quite seeing how to do
> > > this in a clean way. The driver in /drivers/gpu/drm would have to be a
> > > DRM driver that doesn't register a DRM device, but registers a device
> > > in the auxiliary bus for the driver in /drivers/accel to bind to? Or
> > > are you seeing some possibility that would fit better in the current
> > > DRM framework?
> > >
> > > > I think this is the first case we've had of a combo DRM/Accel usecase,
> > > > and so there isn't an existing example to refer you to on how to
> > > > structure things.  I think you are going to be the first example where
> > > > we figure all of this out.
> > >
> > > Yep, I will be grateful for any ideas on how to structure this.
> > >
> > > > On a more implementation note, ioctls for Accel devices should not be
> > > > marked DRM_RENDER_ALLOW.  Seems like your attempt to reuse as much of
> > > > the code as possible trips over this.
> > >
> > > Indeed, thanks.
> > >
> > > Cheers,
> > >
> > > Tomeu
> > >
> > > > -Jeff

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists