[0/3] drm: Use full allocated minor range for DRM

Message

Michał Winiarski Aug. 17, 2022, 11:05 p.m. UTC

64 DRM device nodes is not enough for everyone.
Upgrade it to 512K (which definitely is more than enough).
Additionally - one minor tweak around minor IDR locking.

Michał Winiarski (3):
  drm: Don't reserve minors for control nodes
  drm: Expand max DRM device number to full MINORBITS
  drm: Use mutex for minors

 drivers/gpu/drm/drm_drv.c | 45 ++++++++++++++++++---------------------
 include/drm/drm_file.h    |  1 -
 2 files changed, 21 insertions(+), 25 deletions(-)

Comments

Simon Ser Aug. 19, 2022, 8:16 a.m. UTC | #1

(It seems like the list was dropped in my reply, sorry about that.
Re-adding it now.)

On Thursday, August 18th, 2022 at 14:06, Michał Winiarski <[e-mail]> wrote:

> On Thu, Aug 18, 2022 at 07:39:13AM +0000, Simon Ser wrote:
> 
> > Hm, I'm a bit worried about the user-space implications of this… e.g. libdrm
> > can check for the major/minor to find out the type of a node. Dropping CONTROL
> > from the enum will break that.
> 
> Yeah, but that would only cause problems if there are more than 64 devices in
> the system, and the user-space in question is smart enough to support that.
> 
> IIUC libdrm only looks for 16 devices:
> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.h#L47
> 
> I'm not very familiar with mesa codebase, but I think it has something similar:
> https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c#L52
> 
> I expect other clients to also have something similar (loop over minors, 0-63
> for primary, 128-191 for render).
> 
> So this shouldn't really cause a regression, it's just that "old" userspace
> won't be able to use more devices (but it's also not able to use more devices
> without this series).

Unfortunately I think there are more assumptions all over the place, see e.g.
drmGetMinorType:
https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L986

Also I'm not very found of dropping DRM_NODE_CONTROL from the kernel enum --
this results in DRM_NODE_RENDER=1 in the kernel but DRM_NODE_RENDER=2 in
user-space which sounds pretty error-prone.

> I could go with 0-63 primary, 64-127 empty, 128-191 render, 192-255 primary,
> 256-319 empty, (...)
> But it just seems like a waste to burn 1/3 of minors.

Could potentially work I guess.

> Perhaps it would also be possible to go with:
> 0-63 primary, 64-127 empty, 128-191 render, 192-512K continuous range
> where we distribute minors first-come first-serve, without any link to type (so
> usually we'd get continuous card192, renderD193, and so on)

We would need to re-design drmGetMinorType if we go down this path.

Michał Winiarski Aug. 19, 2022, 9:14 a.m. UTC | #2

On Fri, Aug 19, 2022 at 08:16:07AM +0000, Simon Ser wrote:
> (It seems like the list was dropped in my reply, sorry about that.
> Re-adding it now.)
> 
> On Thursday, August 18th, 2022 at 14:06, Michał Winiarski <[e-mail]> wrote:
> 
> > On Thu, Aug 18, 2022 at 07:39:13AM +0000, Simon Ser wrote:
> > 
> > > Hm, I'm a bit worried about the user-space implications of this… e.g. libdrm
> > > can check for the major/minor to find out the type of a node. Dropping CONTROL
> > > from the enum will break that.
> > 
> > Yeah, but that would only cause problems if there are more than 64 devices in
> > the system, and the user-space in question is smart enough to support that.
> > 
> > IIUC libdrm only looks for 16 devices:
> > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.h#L47
> > 
> > I'm not very familiar with mesa codebase, but I think it has something similar:
> > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c#L52
> > 
> > I expect other clients to also have something similar (loop over minors, 0-63
> > for primary, 128-191 for render).
> > 
> > So this shouldn't really cause a regression, it's just that "old" userspace
> > won't be able to use more devices (but it's also not able to use more devices
> > without this series).
> 
> Unfortunately I think there are more assumptions all over the place, see e.g.
> drmGetMinorType:
> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L986
> 
> Also I'm not very found of dropping DRM_NODE_CONTROL from the kernel enum --
> this results in DRM_NODE_RENDER=1 in the kernel but DRM_NODE_RENDER=2 in
> user-space which sounds pretty error-prone.
> 
> > I could go with 0-63 primary, 64-127 empty, 128-191 render, 192-255 primary,
> > 256-319 empty, (...)
> > But it just seems like a waste to burn 1/3 of minors.
> 
> Could potentially work I guess.
> 
> > Perhaps it would also be possible to go with:
> > 0-63 primary, 64-127 empty, 128-191 render, 192-512K continuous range
> > where we distribute minors first-come first-serve, without any link to type (so
> > usually we'd get continuous card192, renderD193, and so on)
> 
> We would need to re-design drmGetMinorType if we go down this path.

It needs to be changed either way.
Even if we keep reserving 1/3 of minors, drmGetMinorType is still broken (will
return -1, for minors > 191).

Let's respin this with dropping patch 1 and reserving control minors.

-Michał