Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221102203405.1797491-1-ogabbay@kernel.org> <20221102203405.1797491-2-ogabbay@kernel.org>
 <Y2MMCIe5wND2XPqE@kroah.com> <CAFCwf13uLj=P6u6FAcY8M5qAXoaBdb+Ha-TYj0j2FAZnFAPFYg@mail.gmail.com>
 <CAFCwf12yRUG4593ozJMEwaaJBKyWqXTTCjef9O_fzWdQBxVrtw@mail.gmail.com>
 <Y2kAcCu4z2LUMN7u@nvidia.com> <CAFCwf10K-dTu455QfOK8i6thismY-FUN2Rws830EGiqOcGWFgA@mail.gmail.com>
 <Y2kDzPswkKyZyRpS@nvidia.com> <CAFCwf10A=-bj2nR8WasNxyQQ07D24Je04tzKxqv2X_XnA0BUSQ@mail.gmail.com>
 <Y2kRvyR8VrZrO/1H@nvidia.com>
In-Reply-To: <Y2kRvyR8VrZrO/1H@nvidia.com>
From:   Oded Gabbay <ogabbay@kernel.org>
Date:   Mon, 7 Nov 2022 17:53:55 +0200
Message-ID: <CAFCwf12E4K7bpLcDvTtG1mKxoy1JnchXGCKDvcKAayCFvGWXXQ@mail.gmail.com>
Subject: Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a
 new major
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, Arnd Bergmann <arnd@arndb.de>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        John Hubbard <jhubbard@nvidia.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>,
        Jiho Chu <jiho.chu@samsung.com>,
        Daniel Stone <daniel@fooishbar.org>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Jeffrey Hugo <quic_jhugo@quicinc.com>,
        Christoph Hellwig <hch@infradead.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jagan Teki <jagan@amarulasolutions.com>,
        Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>,
        Maciej Kwapulinski <maciej.kwapulinski@linux.intel.com>,
        stanislaw.gruszka@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

To take all of that out of drm.ko and make it a separate kernel module
is a big change, which I don't know if the drm people even want me to
do.

>
> > 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.
>
> > Librarizing parts of DRM sounds nice in theory but the reality is that
> > everything there is interconnected, all the structures are
> > interdependent.
>
> Yes, the kernel is full of stuff that needs improving. Let's not take
> shortcuts.
It's not about shortcuts. It's about a different way to solve this
issue which I don't think is anyway hacky or inappropriate.

>
> > I would have to re-write the entire DRM library to make such a thing
> > work. I don't think this was the intention.
>
> Not necessarily you, whoever someday needs GEM would have to do some
> work.
>
> > The current design makes the accel device an integral part of drm,
> > with very minimal code duplication and without re-writing DRM.
>
> And it smells bad, you can't even make it into a proper module. Who
> knows what other problems will come.
I would argue that if accel is part of drm, it doesn't have to be a
proper module. I don't see that as a hard requirement.
Oded

>
> Jason