| Message ID | 1465930269-7883-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 14, 2016 at 08:50:58PM +0200, Daniel Vetter wrote: > Master-based auth only exists for the legacy/primary drm_minor, hence > there can only be one per device. The goal here is to untangle the > epic dereference games of minor->master and master->minor which is > just massively confusing. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> There can only be one. Looks sane, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Master-based auth only exists for the legacy/primary drm_minor, hence > there can only be one per device. The goal here is to untangle the > epic dereference games of minor->master and master->minor which is > just massively confusing. > Good riddance :-) Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Taking this a step further, have you considered: - having the master under drm_device, then - nuking the drm_file/drm_minor one and adding drm_minor::has_master. The last one will be checked before accessing drm_minor::dev::master. The above should work just fine, right ? -Emil
On Wed, Jun 15, 2016 at 01:50:02PM +0100, Emil Velikov wrote: > On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Master-based auth only exists for the legacy/primary drm_minor, hence > > there can only be one per device. The goal here is to untangle the > > epic dereference games of minor->master and master->minor which is > > just massively confusing. > > > Good riddance :-) > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > Taking this a step further, have you considered: > - having the master under drm_device, then > - nuking the drm_file/drm_minor one and adding drm_minor::has_master. > The last one will be checked before accessing drm_minor::dev::master. > > The above should work just fine, right ? We still need a pointer for the dev->master link. A later patch will simplify that, but entirely embedding doesn't work: drm_master must be free-standing, since when you drop_master all the clients authenticated against you will still be connected to your master. This is to make sure that they loose their authentication when vt switching to another X server. When you reacquire master with set_master, all the clients authenticated against that master are again considered authenticated. But yes, this will be simplified more. -Daniel
On 15 June 2016 at 16:45, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jun 15, 2016 at 01:50:02PM +0100, Emil Velikov wrote: >> On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > Master-based auth only exists for the legacy/primary drm_minor, hence >> > there can only be one per device. The goal here is to untangle the >> > epic dereference games of minor->master and master->minor which is >> > just massively confusing. >> > >> Good riddance :-) >> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> >> >> Taking this a step further, have you considered: >> - having the master under drm_device, then >> - nuking the drm_file/drm_minor one and adding drm_minor::has_master. >> The last one will be checked before accessing drm_minor::dev::master. >> >> The above should work just fine, right ? > > We still need a pointer for the dev->master link. A later patch will > simplify that, but entirely embedding doesn't work: drm_master must be > free-standing, since when you drop_master all the clients authenticated > against you will still be connected to your master. This is to make sure > that they loose their authentication when vt switching to another X > server. When you reacquire master with set_master, all the clients > authenticated against that master are again considered authenticated. > Just to double check: + * Note that master structures are only relevant for the legacy/primary device + * nodes, hence there can only be one per device, not one per drm_minor. In the above does "one per device" reference the "legacy/primary device node" or the "drm_master struct" ? I was thinking about the latter then again, reading your reply I'm not sure any more :-( Or perhaps it is the latter but it should read "one active/associated per device" ? In which case, yes drm_master should be standalone. Can you please shed some light ? Thanks Emil
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8b2582aeaab6..3c01a16bbb77 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -93,7 +93,7 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...) } EXPORT_SYMBOL(drm_ut_debug_printk); -struct drm_master *drm_master_create(struct drm_minor *minor) +struct drm_master *drm_master_create(struct drm_device *dev) { struct drm_master *master; @@ -105,7 +105,7 @@ struct drm_master *drm_master_create(struct drm_minor *minor) spin_lock_init(&master->lock.spinlock); init_waitqueue_head(&master->lock.lock_queue); idr_init(&master->magic_map); - master->minor = minor; + master->dev = dev; return master; } @@ -120,7 +120,7 @@ EXPORT_SYMBOL(drm_master_get); static void drm_master_destroy(struct kref *kref) { struct drm_master *master = container_of(kref, struct drm_master, refcount); - struct drm_device *dev = master->minor->dev; + struct drm_device *dev = master->dev; if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master); diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 2fd4f42b907a..bfbf1381f55d 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -185,7 +185,7 @@ int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv) lockdep_assert_held_once(&dev->master_mutex); /* create a new master */ - fpriv->minor->master = drm_master_create(fpriv->minor); + fpriv->minor->master = drm_master_create(fpriv->minor->dev); if (!fpriv->minor->master) return -ENOMEM; diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 56a9b1cf99d7..f5c1d17fa51f 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -92,7 +92,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -struct drm_master *drm_master_create(struct drm_minor *minor); +struct drm_master *drm_master_create(struct drm_device *dev); /* drm_debugfs.c */ #if defined(CONFIG_DEBUG_FS) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index cd3995d82477..17dcbea4f259 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -379,16 +379,19 @@ struct drm_lock_data { * struct drm_master - drm master structure * * @refcount: Refcount for this master object. - * @minor: Link back to minor char device we are master for. Immutable. + * @dev: Link back to the DRM device * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. * @unique_len: Length of unique field. Protected by drm_global_mutex. * @magic_map: Map of used authentication tokens. Protected by struct_mutex. * @lock: DRI lock information. * @driver_priv: Pointer to driver-private information. + * + * Note that master structures are only relevant for the legacy/primary device + * nodes, hence there can only be one per device, not one per drm_minor. */ struct drm_master { struct kref refcount; - struct drm_minor *minor; + struct drm_device *dev; char *unique; int unique_len; struct idr magic_map;
Master-based auth only exists for the legacy/primary drm_minor, hence there can only be one per device. The goal here is to untangle the epic dereference games of minor->master and master->minor which is just massively confusing. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_drv.c | 6 +++--- drivers/gpu/drm/drm_fops.c | 2 +- drivers/gpu/drm/drm_internal.h | 2 +- include/drm/drmP.h | 7 +++++-- 4 files changed, 10 insertions(+), 7 deletions(-)