| Message ID | 1406129207-1302-4-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: > Lets order things correctly: > ->load() > ->fistopen() > ->open() > ->close() > ->lastclose() > ->unload() > > This doesn't change much as only savage and radeon use ->firstopen() and > they just do map-initialization. Therefore, the global drm mutex makes > sure there cannot be any other f_op between ->open() and ->firstopen(). > However, once we get rid of that lock, we really want ->firstopen() to > initialize the device before ->open() is called. > > Furthermore, this fixes the clean-up path in drm_open(). We currently > don't cleanup the drm_file object if ->firstopen() fails. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ > include/drm/drmP.h | 2 +- > 2 files changed, 66 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 021fe5d..8e73519 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); > > static int drm_open_helper(struct file *filp, struct drm_minor *minor); > > -static int drm_setup(struct drm_device * dev) > +static int drm_firstopen(struct drm_device * dev) All the stuff in here is only for legacy drivers. Imo we should rename this to drm_legacy_setup and hide it harder. Also touching the init ordering for ums drivers is a bit risky, I'd advise against it. firstopen is officially dead for kms driver and really there's nothing legit you can do in there. imx abused until they've switched over to the component framework. I'd just drop this one tbh. -Daniel > { > int ret; > > @@ -66,6 +66,57 @@ static int drm_setup(struct drm_device * dev) > } > > /** > + * drm_legacy_dev_reinit > + * > + * Reinitializes a legacy/ums drm device in it's lastclose function. > + */ > +static void drm_legacy_dev_reinit(struct drm_device *dev) > +{ > + if (drm_core_check_feature(dev, DRIVER_MODESET)) > + return; > + > + dev->sigdata.lock = NULL; > + > + dev->context_flag = 0; > + dev->last_context = 0; > + dev->if_version = 0; > +} > + > +void drm_lastclose(struct drm_device * dev) > +{ > + struct drm_vma_entry *vma, *vma_temp; > + > + DRM_DEBUG("\n"); > + > + if (dev->driver->lastclose) > + dev->driver->lastclose(dev); > + DRM_DEBUG("driver lastclose completed\n"); > + > + if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) > + drm_irq_uninstall(dev); > + > + mutex_lock(&dev->struct_mutex); > + > + drm_agp_clear(dev); > + > + drm_legacy_sg_cleanup(dev); > + > + /* Clear vma list (only built for debugging) */ > + list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { > + list_del(&vma->head); > + kfree(vma); > + } > + > + drm_legacy_dma_takedown(dev); > + > + mutex_unlock(&dev->struct_mutex); > + > + drm_legacy_dev_reinit(dev); > + > + DRM_DEBUG("lastclose completed\n"); > +} > + > +/** > * Open file. > * > * \param inode device inode > @@ -81,31 +132,33 @@ int drm_open(struct inode *inode, struct file *filp) > struct drm_device *dev; > struct drm_minor *minor; > int retcode; > - int need_setup = 0; > > minor = drm_minor_acquire(iminor(inode)); > if (IS_ERR(minor)) > return PTR_ERR(minor); > > dev = minor->dev; > - if (!dev->open_count++) > - need_setup = 1; > > /* share address_space across all char-devs of a single device */ > filp->f_mapping = dev->anon_inode->i_mapping; > > + if (!dev->open_count) { > + retcode = drm_firstopen(dev); > + if (retcode) > + goto err_minor; > + } > + ++dev->open_count; > + > retcode = drm_open_helper(filp, minor); > if (retcode) > goto err_undo; > - if (need_setup) { > - retcode = drm_setup(dev); > - if (retcode) > - goto err_undo; > - } > + > return 0; > > err_undo: > - dev->open_count--; > + if (!--dev->open_count) > + drm_lastclose(dev); > +err_minor: > drm_minor_release(minor); > return retcode; > } > @@ -346,67 +399,6 @@ static void drm_events_release(struct drm_file *file_priv) > } > > /** > - * drm_legacy_dev_reinit > - * > - * Reinitializes a legacy/ums drm device in it's lastclose function. > - */ > -static void drm_legacy_dev_reinit(struct drm_device *dev) > -{ > - if (drm_core_check_feature(dev, DRIVER_MODESET)) > - return; > - > - dev->sigdata.lock = NULL; > - > - dev->context_flag = 0; > - dev->last_context = 0; > - dev->if_version = 0; > -} > - > -/** > - * Take down the DRM device. > - * > - * \param dev DRM device structure. > - * > - * Frees every resource in \p dev. > - * > - * \sa drm_device > - */ > -int drm_lastclose(struct drm_device * dev) > -{ > - struct drm_vma_entry *vma, *vma_temp; > - > - DRM_DEBUG("\n"); > - > - if (dev->driver->lastclose) > - dev->driver->lastclose(dev); > - DRM_DEBUG("driver lastclose completed\n"); > - > - if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) > - drm_irq_uninstall(dev); > - > - mutex_lock(&dev->struct_mutex); > - > - drm_agp_clear(dev); > - > - drm_legacy_sg_cleanup(dev); > - > - /* Clear vma list (only built for debugging) */ > - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { > - list_del(&vma->head); > - kfree(vma); > - } > - > - drm_legacy_dma_takedown(dev); > - > - mutex_unlock(&dev->struct_mutex); > - > - drm_legacy_dev_reinit(dev); > - > - DRM_DEBUG("lastclose completed\n"); > - return 0; > -} > - > -/** > * Release file. > * > * \param inode device inode > @@ -423,7 +415,6 @@ int drm_release(struct inode *inode, struct file *filp) > struct drm_file *file_priv = filp->private_data; > struct drm_minor *minor = file_priv->minor; > struct drm_device *dev = minor->dev; > - int retcode = 0; > > mutex_lock(&drm_global_mutex); > > @@ -541,7 +532,7 @@ int drm_release(struct inode *inode, struct file *filp) > */ > > if (!--dev->open_count) { > - retcode = drm_lastclose(dev); > + drm_lastclose(dev); > if (drm_device_is_unplugged(dev)) > drm_put_dev(dev); > } > @@ -549,7 +540,7 @@ int drm_release(struct inode *inode, struct file *filp) > > drm_minor_release(minor); > > - return retcode; > + return 0; > } > EXPORT_SYMBOL(drm_release); > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index d3d9be6..d1730c5 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1181,7 +1181,7 @@ extern long drm_ioctl(struct file *filp, > unsigned int cmd, unsigned long arg); > extern long drm_compat_ioctl(struct file *filp, > unsigned int cmd, unsigned long arg); > -extern int drm_lastclose(struct drm_device *dev); > +extern void drm_lastclose(struct drm_device *dev); > extern bool drm_ioctl_flags(unsigned int nr, unsigned int *flags); > > /* Device support (drm_fops.h) */ > -- > 2.0.2 >
Hi On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: >> Lets order things correctly: >> ->load() >> ->fistopen() >> ->open() >> ->close() >> ->lastclose() >> ->unload() >> >> This doesn't change much as only savage and radeon use ->firstopen() and >> they just do map-initialization. Therefore, the global drm mutex makes >> sure there cannot be any other f_op between ->open() and ->firstopen(). >> However, once we get rid of that lock, we really want ->firstopen() to >> initialize the device before ->open() is called. >> >> Furthermore, this fixes the clean-up path in drm_open(). We currently >> don't cleanup the drm_file object if ->firstopen() fails. >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ >> include/drm/drmP.h | 2 +- >> 2 files changed, 66 insertions(+), 75 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >> index 021fe5d..8e73519 100644 >> --- a/drivers/gpu/drm/drm_fops.c >> +++ b/drivers/gpu/drm/drm_fops.c >> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); >> >> static int drm_open_helper(struct file *filp, struct drm_minor *minor); >> >> -static int drm_setup(struct drm_device * dev) >> +static int drm_firstopen(struct drm_device * dev) > > All the stuff in here is only for legacy drivers. Imo we should rename > this to drm_legacy_setup and hide it harder. > > Also touching the init ordering for ums drivers is a bit risky, I'd advise > against it. firstopen is officially dead for kms driver and really there's > nothing legit you can do in there. imx abused until they've switched over > to the component framework. > > I'd just drop this one tbh. I did a driver audit when writing this patch, there're only 2 drivers using firstopen: * radeon_cp: sets two fields of dev_private and calls drm_addmap() on those. In lastclose(), it calls drm_rmmap() * savage: sets several fields in dev_private, calls arch_phys_wc_add() for some regions and requests a drm-map for those via drm_addmap(). On lastclose(), it reverts exactly those steps. Taking into account that firstopen() just runs with drm_device context, not with drm_file context, I really think we should be calling it _before_ doing ->open(). I even think the drivers would fail horribly if we don't do this patch and some other user-context sneaks in between both calls (currently impossible thanks to drm_global_mutex). Both ->firstopen() callbacks are fairly trivial. In favor of making the error-paths work in drm_open() I'd really like to get this in. This patch is preparing for my drm_file rework that can make drm_dev_unregister() stop all open files immediately. But I honestly don't care much for UMS drivers, so if you NACK this, I'd just ignore the error code and add a comment that we don't do error-handling for UMS. Cheers David
On Thu, Jul 24, 2014 at 11:31:05AM +0200, David Herrmann wrote: > Hi > > On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: > >> Lets order things correctly: > >> ->load() > >> ->fistopen() > >> ->open() > >> ->close() > >> ->lastclose() > >> ->unload() > >> > >> This doesn't change much as only savage and radeon use ->firstopen() and > >> they just do map-initialization. Therefore, the global drm mutex makes > >> sure there cannot be any other f_op between ->open() and ->firstopen(). > >> However, once we get rid of that lock, we really want ->firstopen() to > >> initialize the device before ->open() is called. > >> > >> Furthermore, this fixes the clean-up path in drm_open(). We currently > >> don't cleanup the drm_file object if ->firstopen() fails. > >> > >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > >> --- > >> drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ > >> include/drm/drmP.h | 2 +- > >> 2 files changed, 66 insertions(+), 75 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > >> index 021fe5d..8e73519 100644 > >> --- a/drivers/gpu/drm/drm_fops.c > >> +++ b/drivers/gpu/drm/drm_fops.c > >> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); > >> > >> static int drm_open_helper(struct file *filp, struct drm_minor *minor); > >> > >> -static int drm_setup(struct drm_device * dev) > >> +static int drm_firstopen(struct drm_device * dev) > > > > All the stuff in here is only for legacy drivers. Imo we should rename > > this to drm_legacy_setup and hide it harder. > > > > Also touching the init ordering for ums drivers is a bit risky, I'd advise > > against it. firstopen is officially dead for kms driver and really there's > > nothing legit you can do in there. imx abused until they've switched over > > to the component framework. > > > > I'd just drop this one tbh. > > I did a driver audit when writing this patch, there're only 2 drivers > using firstopen: > > * radeon_cp: sets two fields of dev_private and calls drm_addmap() on > those. In lastclose(), it calls drm_rmmap() > * savage: sets several fields in dev_private, calls > arch_phys_wc_add() for some regions and requests a drm-map for those > via drm_addmap(). On lastclose(), it reverts exactly those steps. > > Taking into account that firstopen() just runs with drm_device > context, not with drm_file context, I really think we should be > calling it _before_ doing ->open(). I even think the drivers would > fail horribly if we don't do this patch and some other user-context > sneaks in between both calls (currently impossible thanks to > drm_global_mutex). > > Both ->firstopen() callbacks are fairly trivial. In favor of making > the error-paths work in drm_open() I'd really like to get this in. > This patch is preparing for my drm_file rework that can make > drm_dev_unregister() stop all open files immediately. > > But I honestly don't care much for UMS drivers, so if you NACK this, > I'd just ignore the error code and add a comment that we don't do > error-handling for UMS. Yeah, I prefer just labels for dragons instead of poking them ;-) -Daniel
On Thu, Jul 24, 2014 at 5:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: >>> Lets order things correctly: >>> ->load() >>> ->fistopen() >>> ->open() >>> ->close() >>> ->lastclose() >>> ->unload() >>> >>> This doesn't change much as only savage and radeon use ->firstopen() and >>> they just do map-initialization. Therefore, the global drm mutex makes >>> sure there cannot be any other f_op between ->open() and ->firstopen(). >>> However, once we get rid of that lock, we really want ->firstopen() to >>> initialize the device before ->open() is called. >>> >>> Furthermore, this fixes the clean-up path in drm_open(). We currently >>> don't cleanup the drm_file object if ->firstopen() fails. >>> >>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >>> --- >>> drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ >>> include/drm/drmP.h | 2 +- >>> 2 files changed, 66 insertions(+), 75 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >>> index 021fe5d..8e73519 100644 >>> --- a/drivers/gpu/drm/drm_fops.c >>> +++ b/drivers/gpu/drm/drm_fops.c >>> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); >>> >>> static int drm_open_helper(struct file *filp, struct drm_minor *minor); >>> >>> -static int drm_setup(struct drm_device * dev) >>> +static int drm_firstopen(struct drm_device * dev) >> >> All the stuff in here is only for legacy drivers. Imo we should rename >> this to drm_legacy_setup and hide it harder. >> >> Also touching the init ordering for ums drivers is a bit risky, I'd advise >> against it. firstopen is officially dead for kms driver and really there's >> nothing legit you can do in there. imx abused until they've switched over >> to the component framework. >> >> I'd just drop this one tbh. > > I did a driver audit when writing this patch, there're only 2 drivers > using firstopen: > > * radeon_cp: sets two fields of dev_private and calls drm_addmap() on > those. In lastclose(), it calls drm_rmmap() > * savage: sets several fields in dev_private, calls > arch_phys_wc_add() for some regions and requests a drm-map for those > via drm_addmap(). On lastclose(), it reverts exactly those steps. > > Taking into account that firstopen() just runs with drm_device > context, not with drm_file context, I really think we should be > calling it _before_ doing ->open(). I even think the drivers would > fail horribly if we don't do this patch and some other user-context > sneaks in between both calls (currently impossible thanks to > drm_global_mutex). > > Both ->firstopen() callbacks are fairly trivial. In favor of making > the error-paths work in drm_open() I'd really like to get this in. > This patch is preparing for my drm_file rework that can make > drm_dev_unregister() stop all open files immediately. > > But I honestly don't care much for UMS drivers, so if you NACK this, > I'd just ignore the error code and add a comment that we don't do > error-handling for UMS. Well, radeon UMS support is in the kernel deprecated and hasn't been built by default for a while and I doubt anyone has tested it in years. I'm not sure what the current state of savage is. I'm fine either way. Alex > > Cheers > David > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 021fe5d..8e73519 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); static int drm_open_helper(struct file *filp, struct drm_minor *minor); -static int drm_setup(struct drm_device * dev) +static int drm_firstopen(struct drm_device * dev) { int ret; @@ -66,6 +66,57 @@ static int drm_setup(struct drm_device * dev) } /** + * drm_legacy_dev_reinit + * + * Reinitializes a legacy/ums drm device in it's lastclose function. + */ +static void drm_legacy_dev_reinit(struct drm_device *dev) +{ + if (drm_core_check_feature(dev, DRIVER_MODESET)) + return; + + dev->sigdata.lock = NULL; + + dev->context_flag = 0; + dev->last_context = 0; + dev->if_version = 0; +} + +void drm_lastclose(struct drm_device * dev) +{ + struct drm_vma_entry *vma, *vma_temp; + + DRM_DEBUG("\n"); + + if (dev->driver->lastclose) + dev->driver->lastclose(dev); + DRM_DEBUG("driver lastclose completed\n"); + + if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) + drm_irq_uninstall(dev); + + mutex_lock(&dev->struct_mutex); + + drm_agp_clear(dev); + + drm_legacy_sg_cleanup(dev); + + /* Clear vma list (only built for debugging) */ + list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { + list_del(&vma->head); + kfree(vma); + } + + drm_legacy_dma_takedown(dev); + + mutex_unlock(&dev->struct_mutex); + + drm_legacy_dev_reinit(dev); + + DRM_DEBUG("lastclose completed\n"); +} + +/** * Open file. * * \param inode device inode @@ -81,31 +132,33 @@ int drm_open(struct inode *inode, struct file *filp) struct drm_device *dev; struct drm_minor *minor; int retcode; - int need_setup = 0; minor = drm_minor_acquire(iminor(inode)); if (IS_ERR(minor)) return PTR_ERR(minor); dev = minor->dev; - if (!dev->open_count++) - need_setup = 1; /* share address_space across all char-devs of a single device */ filp->f_mapping = dev->anon_inode->i_mapping; + if (!dev->open_count) { + retcode = drm_firstopen(dev); + if (retcode) + goto err_minor; + } + ++dev->open_count; + retcode = drm_open_helper(filp, minor); if (retcode) goto err_undo; - if (need_setup) { - retcode = drm_setup(dev); - if (retcode) - goto err_undo; - } + return 0; err_undo: - dev->open_count--; + if (!--dev->open_count) + drm_lastclose(dev); +err_minor: drm_minor_release(minor); return retcode; } @@ -346,67 +399,6 @@ static void drm_events_release(struct drm_file *file_priv) } /** - * drm_legacy_dev_reinit - * - * Reinitializes a legacy/ums drm device in it's lastclose function. - */ -static void drm_legacy_dev_reinit(struct drm_device *dev) -{ - if (drm_core_check_feature(dev, DRIVER_MODESET)) - return; - - dev->sigdata.lock = NULL; - - dev->context_flag = 0; - dev->last_context = 0; - dev->if_version = 0; -} - -/** - * Take down the DRM device. - * - * \param dev DRM device structure. - * - * Frees every resource in \p dev. - * - * \sa drm_device - */ -int drm_lastclose(struct drm_device * dev) -{ - struct drm_vma_entry *vma, *vma_temp; - - DRM_DEBUG("\n"); - - if (dev->driver->lastclose) - dev->driver->lastclose(dev); - DRM_DEBUG("driver lastclose completed\n"); - - if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET)) - drm_irq_uninstall(dev); - - mutex_lock(&dev->struct_mutex); - - drm_agp_clear(dev); - - drm_legacy_sg_cleanup(dev); - - /* Clear vma list (only built for debugging) */ - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { - list_del(&vma->head); - kfree(vma); - } - - drm_legacy_dma_takedown(dev); - - mutex_unlock(&dev->struct_mutex); - - drm_legacy_dev_reinit(dev); - - DRM_DEBUG("lastclose completed\n"); - return 0; -} - -/** * Release file. * * \param inode device inode @@ -423,7 +415,6 @@ int drm_release(struct inode *inode, struct file *filp) struct drm_file *file_priv = filp->private_data; struct drm_minor *minor = file_priv->minor; struct drm_device *dev = minor->dev; - int retcode = 0; mutex_lock(&drm_global_mutex); @@ -541,7 +532,7 @@ int drm_release(struct inode *inode, struct file *filp) */ if (!--dev->open_count) { - retcode = drm_lastclose(dev); + drm_lastclose(dev); if (drm_device_is_unplugged(dev)) drm_put_dev(dev); } @@ -549,7 +540,7 @@ int drm_release(struct inode *inode, struct file *filp) drm_minor_release(minor); - return retcode; + return 0; } EXPORT_SYMBOL(drm_release); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d3d9be6..d1730c5 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1181,7 +1181,7 @@ extern long drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); extern long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); -extern int drm_lastclose(struct drm_device *dev); +extern void drm_lastclose(struct drm_device *dev); extern bool drm_ioctl_flags(unsigned int nr, unsigned int *flags); /* Device support (drm_fops.h) */
Lets order things correctly: ->load() ->fistopen() ->open() ->close() ->lastclose() ->unload() This doesn't change much as only savage and radeon use ->firstopen() and they just do map-initialization. Therefore, the global drm mutex makes sure there cannot be any other f_op between ->open() and ->firstopen(). However, once we get rid of that lock, we really want ->firstopen() to initialize the device before ->open() is called. Furthermore, this fixes the clean-up path in drm_open(). We currently don't cleanup the drm_file object if ->firstopen() fails. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ include/drm/drmP.h | 2 +- 2 files changed, 66 insertions(+), 75 deletions(-)