On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@xxxxxxxxx wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
