| Message ID | 1349632406-24068-1-git-send-email-dh.herrmann@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi David, Would you have time to document the master_set operation in Documentation/DocBook/drm.tmpl ? :-) On Sunday 07 October 2012 19:53:26 David Herrmann wrote: > We link every DRM "file_priv" to a "drm_master" structure. Currently, the > drmSetMaster() call returns 0 when there is _any_ active master associated > with the "drm_master" structure of the calling "file_priv". This means, > that after drmSetMaster() we are not guaranteed to be DRM-Master and might > not be able to perform mode-setting. > > A way to reproduce this is by starting weston with the DRM backend from > within an X-console (eg., xterm). Because the xserver's "drm_master" is > currently active, weston is assigned to the same master but is inactive > because its VT is inactive and the xserver is still active. But when > "fake-activating" weston, it calls drmSetMaster(). With current behavior > this returns "0/success" and weston thinks that it is DRM-Master, even > though it is not (as the xserver is still DRM-Master). > Expected behavior would be drmSetMaster() to return -EINVAL, because the > xserver is still DRM-Master. This patch changes exactly that. > > The only way this bogus behavior would be useful is for clients to check > whether their associated "drm_master" is currently the active DRM-Master. > But this logic fails if no DRM-Master is currently active at all. Because > then the client itself would become DRM-Master (if it is root) and this > makes this whole thing useles. > > Also note that the second "if-condition": > file_priv->minor->master != file_priv->master > is always true and can be skipped. > > Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> > --- > Note: > Note that this only removes the "if-clause". The code that performs the > setmaster() is actually left unchanged but makes the patch look scarier than > it really is. > > drivers/gpu/drm/drm_stub.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index c236fd2..581e61d 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -221,20 +221,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void > *data, if (!file_priv->master) > return -EINVAL; > > - if (!file_priv->minor->master && > - file_priv->minor->master != file_priv->master) { > - mutex_lock(&dev->struct_mutex); > - file_priv->minor->master = drm_master_get(file_priv->master); > - file_priv->is_master = 1; > - if (dev->driver->master_set) { > - ret = dev->driver->master_set(dev, file_priv, false); > - if (unlikely(ret != 0)) { > - file_priv->is_master = 0; > - drm_master_put(&file_priv->minor->master); > - } > + if (file_priv->minor->master) > + return -EINVAL; > + > + mutex_lock(&dev->struct_mutex); > + file_priv->minor->master = drm_master_get(file_priv->master); > + file_priv->is_master = 1; > + if (dev->driver->master_set) { > + ret = dev->driver->master_set(dev, file_priv, false); > + if (unlikely(ret != 0)) { > + file_priv->is_master = 0; > + drm_master_put(&file_priv->minor->master); > } > - mutex_unlock(&dev->struct_mutex); > } > + mutex_unlock(&dev->struct_mutex); > > return 0; > }
Hi Laurent On Thu, Oct 11, 2012 at 12:35 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi David, > > Would you have time to document the master_set operation in > Documentation/DocBook/drm.tmpl ? :-) I have actually some drafts for "drmSetMaster/drmDropMaster" man-pages for libdrm on my machine. However, I am still waiting for my other man-pages being applied to libdrm (they're pending on the list). The drmSetMaster() man-page does explain the DRM-Master mess in all detail, so I'd like to wait for this being reviewed before adding the same information to kernel-docbook (if that is required at all). Regards David
Hi David, On Thursday 11 October 2012 12:41:43 David Herrmann wrote: > On Thu, Oct 11, 2012 at 12:35 PM, Laurent Pinchart wrote: > > Hi David, > > > > Would you have time to document the master_set operation in > > Documentation/DocBook/drm.tmpl ? :-) > > I have actually some drafts for "drmSetMaster/drmDropMaster" man-pages > for libdrm on my machine. However, I am still waiting for my other > man-pages being applied to libdrm (they're pending on the list). > > The drmSetMaster() man-page does explain the DRM-Master mess in all > detail, so I'd like to wait for this being reviewed before adding the > same information to kernel-docbook Sure, there's no rush. > (if that is required at all). The DocBook documentation (currently) documents the in-kernel APIs only and mostly serves as a document for driver developers. It's currently missing documentation for the master_set operation, so it would be nice if you could add that.
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index c236fd2..581e61d 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -221,20 +221,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, if (!file_priv->master) return -EINVAL; - if (!file_priv->minor->master && - file_priv->minor->master != file_priv->master) { - mutex_lock(&dev->struct_mutex); - file_priv->minor->master = drm_master_get(file_priv->master); - file_priv->is_master = 1; - if (dev->driver->master_set) { - ret = dev->driver->master_set(dev, file_priv, false); - if (unlikely(ret != 0)) { - file_priv->is_master = 0; - drm_master_put(&file_priv->minor->master); - } + if (file_priv->minor->master) + return -EINVAL; + + mutex_lock(&dev->struct_mutex); + file_priv->minor->master = drm_master_get(file_priv->master); + file_priv->is_master = 1; + if (dev->driver->master_set) { + ret = dev->driver->master_set(dev, file_priv, false); + if (unlikely(ret != 0)) { + file_priv->is_master = 0; + drm_master_put(&file_priv->minor->master); } - mutex_unlock(&dev->struct_mutex); } + mutex_unlock(&dev->struct_mutex); return 0; }
We link every DRM "file_priv" to a "drm_master" structure. Currently, the drmSetMaster() call returns 0 when there is _any_ active master associated with the "drm_master" structure of the calling "file_priv". This means, that after drmSetMaster() we are not guaranteed to be DRM-Master and might not be able to perform mode-setting. A way to reproduce this is by starting weston with the DRM backend from within an X-console (eg., xterm). Because the xserver's "drm_master" is currently active, weston is assigned to the same master but is inactive because its VT is inactive and the xserver is still active. But when "fake-activating" weston, it calls drmSetMaster(). With current behavior this returns "0/success" and weston thinks that it is DRM-Master, even though it is not (as the xserver is still DRM-Master). Expected behavior would be drmSetMaster() to return -EINVAL, because the xserver is still DRM-Master. This patch changes exactly that. The only way this bogus behavior would be useful is for clients to check whether their associated "drm_master" is currently the active DRM-Master. But this logic fails if no DRM-Master is currently active at all. Because then the client itself would become DRM-Master (if it is root) and this makes this whole thing useles. Also note that the second "if-condition": file_priv->minor->master != file_priv->master is always true and can be skipped. Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> --- Note: Note that this only removes the "if-clause". The code that performs the setmaster() is actually left unchanged but makes the patch look scarier than it really is. drivers/gpu/drm/drm_stub.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)