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
