On Mon, Nov 7, 2022 at 6:31 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > On Mon, Nov 07, 2022 at 05:53:55PM +0200, Oded Gabbay wrote: > > On Mon, Nov 7, 2022 at 4:10 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > > > > > On Mon, Nov 07, 2022 at 04:02:01PM +0200, Oded Gabbay wrote: > > > > On Mon, Nov 7, 2022 at 3:10 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote: > > > > > > I don't agree with your statement that it should be "a layer over top of DRM". > > > > > > Anything on top of DRM is a device driver. > > > > > > Accel is not a device driver, it is a new type of drm minor / drm driver. > > > > > > > > > > Yeah, I still think this is not the right way, you are getting almost > > > > > nothing from DRM and making everything more complicated in the > > > > > process. > > > > > > > > > > > The only alternative imo to that is to abandon the idea of reusing > > > > > > drm, and just make an independant accel core code. > > > > > > > > > > Not quite really, layer it properly and librarize parts of DRM into > > > > > things accel can re-use so they are not intimately tied to the DRM > > > > > struct device notion. > > > > > > > > > > IMHO this is much better, because accel has very little need of DRM to > > > > > manage a struct device/cdev in the first place. > > > > > > > > > > Jason > > > > I'm not following. How can an accel device be a new type of drm_minor, > > > > if it doesn't have access to all its functions and members ? > > > > > > "drm_minor" is not necessary anymore. Strictly managing minor numbers > > > lost its value years ago when /dev/ was reorganized. Just use > > > dynamic minors fully. > > drm minor is not just about handling minor numbers. It contains the > > entire code to manage devices that register with drm framework (e.g. > > supply callbacks to file operations), manage their lifecycle, > > resources (e.g. automatic free of resources on release), sysfs, > > debugfs, etc. > > This is why you are having such troubles, this is already good library > code. You don't need DRM to wrapper debugfs APIs, for instance. We > have devm, though maybe it is not a good idea, etc > > Greg already pointed out the sysfs was not being done correctly > anyhow. > > I don't think DRM is improving on these core kernel services. Just use > the normal stuff directly. I get what you are saying but if I do all that, then how is this subsystem related to DRM and re-using its code ? (at least at this stage) btw, using the basic stuff directly was my original intention, if you remember the original accel mail thread from July/August. And then we all decided in LPC that we shouldn't do that and instead accel should use the DRM code and just expose a new major+minor for the new drivers. So, something doesn't add up... imo, we need to choose between doing accel either as a small new feature in drm, or as an independent subsystem. I just don't see how I do the former without calling drm code directly and using all its wrappers. > > > > > How will accel device leverage, for example, the GEM code without > > > > being a drm_minor ? > > > > > > Split GEM into a library so it doesn't require that. > > I don't see the advantage of doing that over defining accel as a new > > type of drm minor. > > Making things into smaller libraries is recognized as a far better > kernel approach than trying to make a gigantic wide midlayer that stuffs > itself into everything. LWN called this the "midlayer mistake" and > wrote about the pitfalls a long time ago: > > https://lwn.net/Articles/336262/ > > It is exactly what you are experiencing trying to stretch a > midlayer even further out. > > Jason I'm all for breaking it down to smaller libraries, I completely agree with you. But as you wrote above, why do I even need to use the drm wrappers for the basic stuff ? I'll just call the kernel api directly. And if that's the case then I don't need to rip that code out of the heart of drm and make it a separate module. For GEM (as an example of something less basic) it might be a different story, but we are not there yet. Oded
