| Message ID | 20221011110437.15258-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: remove DRM_MINOR_CONTROL | expand |
On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > --- 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, > }; This makes me uncomfortable: this enum no longer matches DRM_NODE_* in libdrm.
Am 11.10.22 um 13:39 schrieb Simon Ser: > On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > >> --- 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, >> }; > This makes me uncomfortable: this enum no longer matches DRM_NODE_* in > libdrm. Ah! There it was! I was remembering in the back of my head that we had somehow used this in libdrm as well, but couldn't really get where exactly. But I don't really see a problem here. The control nodes are identified by name and we don't expose them for quite some time now without any negative impact. Even the minor number distribution stays the same. So what bad can come from this? Thanks, Christian.
On Tue, Oct 11, 2022 at 01:55:01PM +0200, Christian König wrote: > Am 11.10.22 um 13:39 schrieb Simon Ser: > > On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > > > > --- 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, > > > }; > > This makes me uncomfortable: this enum no longer matches DRM_NODE_* in > > libdrm. > > Ah! There it was! I was remembering in the back of my head that we had > somehow used this in libdrm as well, but couldn't really get where exactly. > > But I don't really see a problem here. The control nodes are identified by > name and we don't expose them for quite some time now without any negative > impact. > > Even the minor number distribution stays the same. So what bad can come from > this? > > Thanks, > Christian. > I proposed something similar in: https://lore.kernel.org/dri-devel/20220817230600.272790-1-michal.winiarski@intel.com/ except acompanied by expanding the minor pool to accomodate more than 128 devices: And after receiving similar feedback, that eventually evolved into leaving the "legacy minors" alone, and changing the rules only for cases where we have more than 64 devices (when we run out of the "legacy minors"). Perhaps something like this: https://lore.kernel.org/dri-devel/20220911211443.581481-1-michal.winiarski@intel.com/ Would work for your usecase as well? -Michał
Am 25.10.22 um 15:59 schrieb Michał Winiarski: > On Tue, Oct 11, 2022 at 01:55:01PM +0200, Christian König wrote: >> Am 11.10.22 um 13:39 schrieb Simon Ser: >>> On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: >>> >>>> --- 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, >>>> }; >>> This makes me uncomfortable: this enum no longer matches DRM_NODE_* in >>> libdrm. >> Ah! There it was! I was remembering in the back of my head that we had >> somehow used this in libdrm as well, but couldn't really get where exactly. >> >> But I don't really see a problem here. The control nodes are identified by >> name and we don't expose them for quite some time now without any negative >> impact. >> >> Even the minor number distribution stays the same. So what bad can come from >> this? >> >> Thanks, >> Christian. >> > I proposed something similar in: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20220817230600.272790-1-michal.winiarski%40intel.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C085831b6e1b647ca1dbd08dab6911b4b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638023031607291573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=56FxZ4UThGlbJ0ut8biicsYtIvtTZ8RGHISJqe%2BXixY%3D&reserved=0 > except acompanied by expanding the minor pool to accomodate more than > 128 devices: > > And after receiving similar feedback, that eventually evolved into > leaving the "legacy minors" alone, and changing the rules only for cases > where we have more than 64 devices (when we run out of the "legacy > minors"). > > Perhaps something like this: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20220911211443.581481-1-michal.winiarski%40intel.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C085831b6e1b647ca1dbd08dab6911b4b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638023031607291573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=dFRTx0%2Fi7a4aps57JWGtEJ6GoW5IfI5CQjFkig4KFnA%3D&reserved=0 > Would work for your usecase as well? We don't desperately need the additional minor numbers, this was merely just a cleanup to remove an unused define. Christian. > > -Michał
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8214a0b1ab7f..d81783f43452 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -126,8 +126,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 d780fd151789..a3be533e99e0 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, };
Not used any more. This makes room for up to 128 DRM devices. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_drv.c | 4 ++-- include/drm/drm_file.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)