Re: [PATCH v1] misc: fastrpc: Move fastrpc driver to misc/fastrpc/

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 21, 2024 at 10:48:27PM +0300, Dmitry Baryshkov wrote:
> On Fri, 21 Jun 2024 at 18:52, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Jun 21, 2024 at 09:40:09AM -0600, Jeffrey Hugo wrote:
> > > On 6/21/2024 5:19 AM, Dmitry Baryshkov wrote:
> > > > On Fri, 21 Jun 2024 at 09:19, Bjorn Andersson <andersson@kernel.org> wrote:
> > > > >
> > > > > On Wed, Jun 12, 2024 at 09:28:39PM GMT, Dmitry Baryshkov wrote:
> > > > > > On Wed, Jun 12, 2024 at 12:17:28PM +0530, Ekansh Gupta wrote:
> > > > > > > Move fastrpc.c from misc/ to misc/fastrpc/. New C files are planned
> > > > > > > to be added for PD notifications and other missing features. Adding
> > > > > > > and maintaining new files from within fastrpc directory would be easy.
> > > > > > >
> > > > > > > Example of feature that is being planned to be introduced in a new C
> > > > > > > file:
> > > > > > > https://lore.kernel.org/all/20240606165939.12950-6-quic_ekangupt@quicinc.com/
> > > > > > >
> > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > ---
> > > > > > >   MAINTAINERS                          |  2 +-
> > > > > > >   drivers/misc/Kconfig                 | 13 +------------
> > > > > > >   drivers/misc/Makefile                |  2 +-
> > > > > > >   drivers/misc/fastrpc/Kconfig         | 16 ++++++++++++++++
> > > > > > >   drivers/misc/fastrpc/Makefile        |  2 ++
> > > > > > >   drivers/misc/{ => fastrpc}/fastrpc.c |  0
> > > > > > >   6 files changed, 21 insertions(+), 14 deletions(-)
> > > > > > >   create mode 100644 drivers/misc/fastrpc/Kconfig
> > > > > > >   create mode 100644 drivers/misc/fastrpc/Makefile
> > > > > > >   rename drivers/misc/{ => fastrpc}/fastrpc.c (100%)
> > > > > >
> > > > > > Please consider whether it makes sense to move to drivers/accel instead
> > > > > > (and possibly writing a better Kconfig entry, specifying that the driver
> > > > > > is to be used to offload execution to the DSP).
> > > > > >
> > > > >
> > > > > Wouldn't this come with the expectation of following the ABIs of
> > > > > drivers/accel and thereby breaking userspace?
> > > >
> > > > As I wrote earlier, that depends on the accel/ maintainers decision,
> > > > whether it's acceptable to have non-DRM_ACCEL code underneath.
> > > > But at least I'd try doing that on the grounds of keeping the code at
> > > > the proper place in the drivers/ tree, raising awareness of the
> > > > FastRPC, etc.
> > > > For example current fastrpc driver bypasses dri-devel reviews, while
> > > > if I remember correctly, at some point it was suggested that all
> > > > dma-buf-handling drivers should also notify the dri-devel ML.
> > > >
> > > > Also having the driver under drivers/accels makes it possible and
> > > > logical to  implement DRM_ACCEL uAPI at some point. In the ideal world
> > > > we should be able to declare existing FastRPC uAPI as legacy /
> > > > deprecated / backwards compatibility only and migrate to the
> > > > recommended uAPI approach, which is DRM_ACCEL.
> > > >
> > >
> > > I suspect Vetter/Airlie need to be involved in this.
> > >
> > > Its my understanding that accelerator drivers are able to reside in misc as
> > > long as there is no use of dma-buf.  Use of dma-buf means they need to be in
> > > drm/accel.
> > >
> > > There is precedent for moving a driver from misc to accel (HabanaLabs).
> > >
> > > Right now, I'm not aware that fastRPC meets the requirements for drm/accel.
> > > There is an open source userspace driver, but I'm not aware of an open
> > > source compiler.  From what I know of the architecture, it should be
> > > possible to utilize upstream LLVM to produce one.
> >
> > Yeah so fastrpc is one of the reasons why I've added a dma_buf regex match
> > to MAINTAINERS, and given this move has shown up here on dri-devel that
> > seems to work.
> >
> > But also, it slipped through, can't break uapi, so I just pretend it's not
> > really there :-)
> >
> > That aside, going forward it might make sense to look into drivers/accel,
> > and also going forward new dma_buf uapi will be reviewed to fairly
> > stringent standards. We're not going to impose the dri-devel userspace
> > rules on everyone, each subsystem tends to know what's best in their
> > ecosystem. But if something just ends up in misc so it can avoid the drm
> > or accel rules (and I think media is also pretty much on the same page
> > nowadays), then expect some serious heat ...
> 
> After discussing this on #dri-devel, I'm going to retract my
> suggestion of moving the driver to drivers/accel/, unless there is an
> actual interest in moving to drm_accel.h style of uAPI.
> 
> It should still be noted that there is a strong recommendation to
> start from scratch and to use DRM / accel uAPI, either using the
> existing driver for the legacy platforms or dropping it completely.
> When the fastrpc driver was started by Qualcomm engineers, there was
> no standard method of implementing the accel drivers. Since 1st of
> November 2022 we have drm_accel.h.

Yeah, if fastrpc just keeps growing the story will completely different.

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch