Re: [PATCH 03/12] drm: call ->firstopen() before ->open()

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch