[PATCH] drm: Don't reserve minors for control nodes

[Michał Winiarski]

Quoting Daniel Vetter (2020-04-21 15:13:34)
> On Tue, Apr 21, 2020 at 2:50 PM Michał Winiarski <michal at hardline.pl> wrote:
> >
> > From: Michał Winiarski <michal.winiarski at intel.com>
> >
> > Control nodes are no longer with us.
> > While we still need to preserve render nodes numbering, there's no need
> > to reserve the range formerly used for control. Let's repurpose it to be
> > used by primary and remove control remains from the code entirely.
> >
> > References: 0d49f303e8a7 ("drm: remove all control node code")
> > References: c9ac371d4b59 ("drm: Fix render node numbering regression from control node removal.")
> > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Eric Anholt <eric at anholt.net>
> > Cc: Marcin Bernatowicz <marcin.bernatowicz at intel.com>
> > Cc: Sean Paul <seanpaul at chromium.org>
> 
> So someone plugged in 256k gpus in a single machine and we've run out
> of minors? Or why do we need this?
> 
> (There's 20 bits allocated to the minor, and we pre-reserve 2 bits for
> the different flavours, which this patch reduces to 1 bit).

64 primary nodes is the limit right now. We're probably going to have to tackle
the 256k GPUs usecase in the future, but perhaps not today :)

> I'm asking since we might have some userspace somewhere which
> hardcodes this and gets surprised. And I kinda don't want to audit the
> world ... So I'm wondering what the motivation here is.
> -Daniel

You mean hardcodes the range (n in the snippet below)?
Even if userspace would do the simplest approach of looking for drm device, so
something like (which btw is why c9ac371d4b59 was introduced):

n = 64;
/* primary */
for (i = 0; i < n; i++) {
        sprintf(foo, "/dev/dri/card%d", i);
        fd = open(foo);
        /* check whether this is the device we're looking for */
}

/* render */
for (i = 128; i < n; i++) {
        sprintf(foo, "/dev/dri/renderD%d", i);
        fd = open(foo);
        /* check whether this is the device we're looking for */
}

We're changing "n" to 128 on DRM side - worst case is userspace won't find its
device, but it wouldn't find it on older kernels anyway.

-Michał

> 
> > ---
> >  drivers/gpu/drm/drm_drv.c | 4 ++--
> >  include/drm/drm_file.h    | 1 -
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index c15c9b4540e1..366a760bbc29 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -124,8 +124,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> >         spin_lock_irqsave(&drm_minor_lock, flags);
> >         r = idr_alloc(&drm_minors_idr,
> >                       NULL,
> > -                     64 * type,
> > -                     64 * (type + 1),
> > +                     128 * type,
> > +                     128 * (type + 1),
> >                       GFP_NOWAIT);
> >         spin_unlock_irqrestore(&drm_minor_lock, flags);
> >         idr_preload_end();
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 716990bace10..45e6dae69293 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -54,7 +54,6 @@ struct file;
> >   */
> >  enum drm_minor_type {
> >         DRM_MINOR_PRIMARY,
> > -       DRM_MINOR_CONTROL,
> >         DRM_MINOR_RENDER,
> >  };
> >
> > --
> > 2.26.0
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch