[Intel-gfx] [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Daniel Vetter]

On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi at intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>
> 
> With runtime PM enabled, we need to make sure that all HW access
> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> HW hangs. Ex. A hang is seen if display register is accessed on
> BYT while display power island is power gated.
> 
> This patch is covering all the IOCTLs with get/put.
> TODO: limit runtime_get/put to IOCTLs that accesses HW
> 
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>

Nack on the concept. We need to have get/put calls for the individual
functional blocks of the hw, which then in turn (if it's not the top-level
power domain) need to grab references to the next up power domain.
Splattering unconditional get/puts over all driver entry points is bad
style and imo also too fragile.

Also, with Paulos final runtime pm/pc8 unification patches and Imre's
display power well patches for byt we should have full coverage already.
Have you looked at the work of these too?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c   | 21 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_ioc32.c | 11 +++++++++--
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 82c4605..80965be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -845,6 +845,25 @@ i915_pci_remove(struct pci_dev *pdev)
>  	drm_put_dev(dev);
>  }
>  
> +static long i915_unlocked_ioctl(struct file *filp,
> +	      unsigned int cmd, unsigned long arg)
> +{
> +	struct drm_file *file_priv = filp->private_data;
> +	struct drm_device *dev = file_priv->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	/* Don't do anything if device is not ready */
> +	if (drm_device_is_unplugged(dev))
> +		return -ENODEV;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	ret = drm_ioctl(filp, cmd, arg);
> +	intel_runtime_pm_put(dev_priv);
> +
> +	return ret;
> +}
> +
>  static int i915_pm_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -970,7 +989,7 @@ static const struct file_operations i915_driver_fops = {
>  	.owner = THIS_MODULE,
>  	.open = drm_open,
>  	.release = drm_release,
> -	.unlocked_ioctl = drm_ioctl,
> +	.unlocked_ioctl = i915_unlocked_ioctl,
>  	.mmap = drm_gem_mmap,
>  	.poll = drm_poll,
>  	.read = drm_read,
> diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c
> index 3c59584..bfc773e 100644
> --- a/drivers/gpu/drm/i915/i915_ioc32.c
> +++ b/drivers/gpu/drm/i915/i915_ioc32.c
> @@ -201,12 +201,17 @@ static drm_ioctl_compat_t *i915_compat_ioctls[] = {
>   */
>  long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	struct drm_file *file_priv = filp->private_data;
> +	struct drm_device *dev = file_priv->minor->dev;
>  	unsigned int nr = DRM_IOCTL_NR(cmd);
>  	drm_ioctl_compat_t *fn = NULL;
>  	int ret;
>  
> -	if (nr < DRM_COMMAND_BASE)
> -		return drm_compat_ioctl(filp, cmd, arg);
> +	intel_runtime_pm_get(dev->dev_private);
> +	if (nr < DRM_COMMAND_BASE) {
> +		ret = drm_compat_ioctl(filp, cmd, arg);
> +		goto out;
> +	}
>  
>  	if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(i915_compat_ioctls))
>  		fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];
> @@ -216,6 +221,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	else
>  		ret = drm_ioctl(filp, cmd, arg);
>  
> +out:
> +	intel_runtime_pm_put(dev->dev_private);
>  	return ret;
>  }
>  #endif
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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