Re: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices

[Jiho Chu]

On Wed, 26 Oct 2022 09:38:13 +0300
Oded Gabbay <ogabbay@xxxxxxxxxx> wrote:

> On Tue, Oct 25, 2022 at 9:43 AM Jiho Chu <jiho.chu@xxxxxxxxxxx> wrote:
> >
> >
> > On Sun, 23 Oct 2022 00:46:22 +0300
> > Oded Gabbay <ogabbay@xxxxxxxxxx> wrote:
> >
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index b58ffb1433d6..c13701a8d4be 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -56,6 +56,9 @@ MODULE_LICENSE("GPL and additional rights");
> > >  static DEFINE_SPINLOCK(drm_minor_lock);
> > >  static struct idr drm_minors_idr;
> > >
> > > +static DEFINE_SPINLOCK(accel_minor_lock);
> > > +static struct idr accel_minors_idr;
> > > +
> > >  /*
> > >   * If the drm core fails to init for whatever reason,
> > >   * we should prevent any drivers from registering with it.
> > > @@ -94,6 +97,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
> > >               return &dev->primary;
> > >       case DRM_MINOR_RENDER:
> > >               return &dev->render;
> > > +     case DRM_MINOR_ACCEL:
> > > +             return &dev->accel;
> > >       default:
> > >               BUG();
> > >       }
> > > @@ -108,9 +113,15 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > >
> > >       put_device(minor->kdev);
> > >
> > > -     spin_lock_irqsave(&drm_minor_lock, flags);
> > > -     idr_remove(&drm_minors_idr, minor->index);
> > > -     spin_unlock_irqrestore(&drm_minor_lock, flags);
> > > +     if (minor->type == DRM_MINOR_ACCEL) {
> > > +             spin_lock_irqsave(&accel_minor_lock, flags);
> > > +             idr_remove(&accel_minors_idr, minor->index);
> > > +             spin_unlock_irqrestore(&accel_minor_lock, flags);
> > > +     } else {
> > > +             spin_lock_irqsave(&drm_minor_lock, flags);
> > > +             idr_remove(&drm_minors_idr, minor->index);
> > > +             spin_unlock_irqrestore(&drm_minor_lock, flags);
> > > +     }
> > >  }
> > >
> > >  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> > > @@ -127,13 +138,23 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> > >       minor->dev = dev;
> > >
> > >       idr_preload(GFP_KERNEL);
> > > -     spin_lock_irqsave(&drm_minor_lock, flags);
> > > -     r = idr_alloc(&drm_minors_idr,
> > > -                   NULL,
> > > -                   64 * type,
> > > -                   64 * (type + 1),
> > > -                   GFP_NOWAIT);
> > > -     spin_unlock_irqrestore(&drm_minor_lock, flags);
> > > +     if (type == DRM_MINOR_ACCEL) {
> > > +             spin_lock_irqsave(&accel_minor_lock, flags);
> > > +             r = idr_alloc(&accel_minors_idr,
> > > +                     NULL,
> > > +                     64 * (type - DRM_MINOR_ACCEL),
> > > +                     64 * (type - DRM_MINOR_ACCEL + 1),
> > > +                     GFP_NOWAIT);
> > > +             spin_unlock_irqrestore(&accel_minor_lock, flags);
> > > +     } else {
> > > +             spin_lock_irqsave(&drm_minor_lock, flags);
> > > +             r = idr_alloc(&drm_minors_idr,
> > > +                     NULL,
> > > +                     64 * type,
> > > +                     64 * (type + 1),
> > > +                     GFP_NOWAIT);
> > > +             spin_unlock_irqrestore(&drm_minor_lock, flags);
> > > +     }
> >
> > Hi,
> > There are many functions which checks drm type and decides its behaviors. It's good to
> > re-use exiting codes, but accel devices use totally different major/minor, and so it needs to be moved to
> > /drvier/accel/ (maybe later..). How about seperating functions for alloc/release minor (accel_minor_alloc..)?
> > also, for others which have drm type related codes.
> My feeling was moving the minor code handling to a different file (in
> addition to moving the major code handling) will cause too much
> duplication.
> My main theme is that an accel minor is another minor in drm, even if
> a bit different. i.e. It uses the same drm_minor structure.
> The driver declares he wants to use this minor using a drm driver feature flag.
> imo, all of that indicates the code should be inside drm.
> >
> >
> >
> >
> > > @@ -607,6 +652,14 @@ static int drm_dev_init(struct drm_device *dev,
> > >       /* no per-device feature limits by default */
> > >       dev->driver_features = ~0u;
> > >
> > > +     if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) &&
> > > +                             (drm_core_check_feature(dev, DRIVER_RENDER) ||
> > > +                             drm_core_check_feature(dev, DRIVER_MODESET))) {
> > > +
> > > +             DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> >
> > It's fine for the device only for acceleration, but can't graphic devices have acceleration feature?
> Of course they can :) In that case, and if they want to expose an
> accel device char, they should write an accel driver and connect it to
> their main graphics driver via auxiliary bus.
> 
> I could have added two flags - compute_accel, and compute_accel_only
> (similar to a patch that was sent to add render only flag), but imo it
> would make the code more convoluted. I prefer the clean separation and
> using standard auxiliary bus.
> 
> Thanks,
> Oded
> 

I understood. Seperation would be good as you mentioned in other mail.
This subsystem would be better choice for acceleration only devices, who need some features
of drm, but deoesnot want to include whole graphics related considerations.
I'll prepare Samsung's NPU driver using this after your reference driver is presented (maybe habana').

Thanks.
Jiho Chu