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. 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. Thanks, Oded. p.s. Please only use bottom-posting when replying, thanks :) > > 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
