| Message ID | 1407416655-15981-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Aug 7, 2014 at 3:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Despite the claims of > > commit 48ba813701eb14b3008edefef4a0789b328e278c > Author: David Herrmann <dh.herrmann@gmail.com> > Date: Tue Jul 22 18:46:09 2014 +0200 > > drm: drop redundant drm_file->is_master > > drm_file->is_master is not synomous with having drm_file->master == > drm_file->minor->master. This is because drm_file->master is the same > for all drm_files of the same generation and so when there is a master, > every drm_file believes itself to be the master. Confusion ensues and > things go pear shaped when one file is closed and there is no master > anymore. Uagh, embarrassing. A revert is fine with me, but I'll try to review your patch once I get home. Thanks David > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82283 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_drv.c | 5 ++--- > drivers/gpu/drm/drm_fops.c | 12 ++++++------ > include/drm/drmP.h | 3 ++- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index a1863d8..76cdb51 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -198,6 +198,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, > if (unlikely(ret != 0)) > drm_master_put(&file_priv->minor->master); > } > + file_priv->is_master = ret == 0; > > out_unlock: > mutex_unlock(&dev->master_mutex); > @@ -213,13 +214,11 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > if (!drm_is_master(file_priv)) > goto out_unlock; > > - if (!file_priv->minor->master) > - goto out_unlock; > - > ret = 0; > if (dev->driver->master_drop) > dev->driver->master_drop(dev, file_priv, false); > drm_master_put(&file_priv->minor->master); > + file_priv->is_master = false; > > out_unlock: > mutex_unlock(&dev->master_mutex); > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index c0166ba..083f7b9 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -196,6 +196,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) > > /* take another reference for the copy in the local file priv */ > priv->master = drm_master_get(priv->minor->master); > + priv->is_master = true; > priv->authenticated = 1; > > if (dev->driver->master_create) { > @@ -434,12 +435,11 @@ int drm_release(struct inode *inode, struct file *filp) > } > mutex_unlock(&dev->struct_mutex); > > - if (file_priv->minor->master == file_priv->master) { > - /* drop the reference held my the minor */ > - if (dev->driver->master_drop) > - dev->driver->master_drop(dev, file_priv, true); > - drm_master_put(&file_priv->minor->master); > - } > + /* drop the reference held my the minor */ > + if (dev->driver->master_drop) > + dev->driver->master_drop(dev, file_priv, true); > + drm_master_put(&file_priv->minor->master); > + file_priv->is_master = false; > } > > /* drop the master reference held by the file priv */ > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 0ffce5c..70a6598 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -378,6 +378,7 @@ struct drm_prime_file_private { > > /** File private data */ > struct drm_file { > + bool is_master:1; > unsigned authenticated :1; > /* true when the client has asked us to expose stereo 3D mode flags */ > unsigned stereo_allowed :1; > @@ -1177,7 +1178,7 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv) > */ > static inline bool drm_is_master(const struct drm_file *file) > { > - return file->master && file->master == file->minor->master; > + return file->is_master; > } > > /******************************************************************/ > -- > 1.9.1 >
On 8 August 2014 02:00, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Thu, Aug 7, 2014 at 3:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Despite the claims of >> >> commit 48ba813701eb14b3008edefef4a0789b328e278c >> Author: David Herrmann <dh.herrmann@gmail.com> >> Date: Tue Jul 22 18:46:09 2014 +0200 >> >> drm: drop redundant drm_file->is_master >> >> drm_file->is_master is not synomous with having drm_file->master == >> drm_file->minor->master. This is because drm_file->master is the same >> for all drm_files of the same generation and so when there is a master, >> every drm_file believes itself to be the master. Confusion ensues and >> things go pear shaped when one file is closed and there is no master >> anymore. > > Uagh, embarrassing. A revert is fine with me, but I'll try to review > your patch once I get home. > At this point I'll just revert, though I do like the wrapper instead of checking the flag, but its late in the day. Dave.
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index a1863d8..76cdb51 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -198,6 +198,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, if (unlikely(ret != 0)) drm_master_put(&file_priv->minor->master); } + file_priv->is_master = ret == 0; out_unlock: mutex_unlock(&dev->master_mutex); @@ -213,13 +214,11 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, if (!drm_is_master(file_priv)) goto out_unlock; - if (!file_priv->minor->master) - goto out_unlock; - ret = 0; if (dev->driver->master_drop) dev->driver->master_drop(dev, file_priv, false); drm_master_put(&file_priv->minor->master); + file_priv->is_master = false; out_unlock: mutex_unlock(&dev->master_mutex); diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c0166ba..083f7b9 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -196,6 +196,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master); + priv->is_master = true; priv->authenticated = 1; if (dev->driver->master_create) { @@ -434,12 +435,11 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->struct_mutex); - if (file_priv->minor->master == file_priv->master) { - /* drop the reference held my the minor */ - if (dev->driver->master_drop) - dev->driver->master_drop(dev, file_priv, true); - drm_master_put(&file_priv->minor->master); - } + /* drop the reference held my the minor */ + if (dev->driver->master_drop) + dev->driver->master_drop(dev, file_priv, true); + drm_master_put(&file_priv->minor->master); + file_priv->is_master = false; } /* drop the master reference held by the file priv */ diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0ffce5c..70a6598 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -378,6 +378,7 @@ struct drm_prime_file_private { /** File private data */ struct drm_file { + bool is_master:1; unsigned authenticated :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1; @@ -1177,7 +1178,7 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv) */ static inline bool drm_is_master(const struct drm_file *file) { - return file->master && file->master == file->minor->master; + return file->is_master; } /******************************************************************/
Despite the claims of commit 48ba813701eb14b3008edefef4a0789b328e278c Author: David Herrmann <dh.herrmann@gmail.com> Date: Tue Jul 22 18:46:09 2014 +0200 drm: drop redundant drm_file->is_master drm_file->is_master is not synomous with having drm_file->master == drm_file->minor->master. This is because drm_file->master is the same for all drm_files of the same generation and so when there is a master, every drm_file believes itself to be the master. Confusion ensues and things go pear shaped when one file is closed and there is no master anymore. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82283 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_drv.c | 5 ++--- drivers/gpu/drm/drm_fops.c | 12 ++++++------ include/drm/drmP.h | 3 ++- 3 files changed, 10 insertions(+), 10 deletions(-)