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

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

[Michał Winiarski]

From: Michał Winiarski <michal.winiarski@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@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
Cc: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Apr 21, 2020 at 2:50 PM Michał Winiarski <michal@hardline.pl> wrote:
>
> From: Michał Winiarski <michal.winiarski@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@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
> Cc: Sean Paul <seanpaul@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).

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

> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@hardline.pl> wrote:
> >
> > From: Michał Winiarski <michal.winiarski@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@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
> > Cc: Sean Paul <seanpaul@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Apr 21, 2020 at 4:34 PM Michał Winiarski <michal@hardline.pl> wrote:
>
> Quoting Daniel Vetter (2020-04-21 15:13:34)
> > On Tue, Apr 21, 2020 at 2:50 PM Michał Winiarski <michal@hardline.pl> wrote:
> > >
> > > From: Michał Winiarski <michal.winiarski@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@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
> > > Cc: Sean Paul <seanpaul@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.

I was more worried about userspace computing render node minor as a
fixed offset of the primary node minor number. Iirc we have code like
that all over. But reading your patch again, looks all ok, you're
adjusting offests to match again.

Wrt supporting more than 64 devices. Once you've exhausted those we'll
continue allocating more at 256 minor for the next primary, or at
least that's how I thought this code works. There's 2^^20 minors
available for the drm device. If you have userspace somewhere that
goes boom already after 64 devices then it'll just go boom after 128
too. Not really sustainable, imo better to fix your userspace that
somehow assumes minors are continuous. And unfortunately (because of
that hard-coded offset between render and primary nodes) we cannot fix
this in the kernel for real for anything remotely resembling
reasonable use-cases. And yes with servers and virtual devices and a
bunch of test device drivers I think 128 drm drivers isn't any less
unrealistic than just 64.
-Daniel

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel