Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major

From: Jason Gunthorpe <jgg@nvidia.com>
To: Oded Gabbay <ogabbay@kernel.org>
Cc: dri-devel@lists.freedesktop.org,
	Maciej Kwapulinski <maciej.kwapulinski@linux.intel.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Christoph Hellwig <hch@infradead.org>,
	Jagan Teki <jagan@amarulasolutions.com>,
	John Hubbard <jhubbard@nvidia.com>,
	stanislaw.gruszka@intel.com,
	Jeffrey Hugo <quic_jhugo@quicinc.com>,
	Arnd Bergmann <arnd@arndb.de>, Jiho Chu <jiho.chu@samsung.com>,
	Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>,
	Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major
Date: Mon, 7 Nov 2022 12:30:54 -0400	[thread overview]
Message-ID: <Y2kyvswuCsO0x1/2@nvidia.com> (raw)
In-Reply-To: <CAFCwf12E4K7bpLcDvTtG1mKxoy1JnchXGCKDvcKAayCFvGWXXQ@mail.gmail.com>

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@nvidia.com> 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@nvidia.com> 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.

> > > 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