Oded, Dave, Do you have an opinion on this? Thanks, Tomeu On Fri, Apr 26, 2024 at 8:10 AM Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx> wrote: > > On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > > Cc: Oded Gabbay <ogabbay@xxxxxxxxxx> > > > > 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
