[Intel-gfx] [RFC 5/6] drm/i915: device specific runtime suspend/resume

[Daniel Vetter]

On Wed, Jan 22, 2014 at 05:34:21PM +0530, naresh.kumar.kachhi at intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>
> 
> we might need special handling for different platforms.
> for ex. baytrail. To handle this this patch creates function
> pointers for runtime suspend/resume which are assigned
> during driver load time
> 
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi at intel.com>

Again NAK on principles: vtables always have a cost, so before we add a
new one we need to demonstrate the need. Which means 2-3 different
implementations for the vfunc must exist. Sometimes some of these are
still embargo'ed in the -internal tree (like with the ppgtt refactor a
year ago) and that's ok.

The other reason I don't like this is that thus far the driver
load/teardown and suspend/resume sequences are generic. There are tricky
(sometimes really tricky) dependcies, but that's the design thus far. So
if we want to switch the design to per-platform functions we need good
reasons for that switch in general. Which means likely the same issues
will crop up in the normal suspend/resume and driver load/teardown paths.
Which leaves me wondering why we don't have this here.

Finally we've been bitten countless times through superflous differences
and duplicated codepaths between normal operation, suspend/resume, driver
load/teardown and now runtime pm. So again deviating from a shared code
pattern need excellent justification. Yes I know that atm pc8 has its own
stuff, and I expect this to hurt us eventually ;-)

Looking at this patch I don't see any of this.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++---------------
>  drivers/gpu/drm/i915/i915_drv.h |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 80965be..dc1cfab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -935,19 +935,10 @@ static int i915_runtime_suspend(struct device *device)
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
> -	i915_gem_release_all_mmaps(dev_priv);
> -
> -	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> -	dev_priv->pm.suspended = true;
> -
> -	/*
> -	 * current versions of firmware which depend on this opregion
> -	 * notification have repurposed the D1 definition to mean
> -	 * "runtime suspended" vs. what you would normally expect (D3)
> -	 * to distinguish it from notifications that might be sent
> -	 * via the suspend path.
> -	 */
> -	intel_opregion_notify_adapter(dev, PCI_D1);
> +	if (dev_priv->pm.funcs.runtime_suspend)
> +		dev_priv->pm.funcs.runtime_suspend(dev_priv);
> +	else
> +		DRM_ERROR("Device does not have runtime functions");
>  
>  	return 0;
>  }
> @@ -962,8 +953,10 @@ static int i915_runtime_resume(struct device *device)
>  
>  	DRM_DEBUG_KMS("Resuming device\n");
>  
> -	intel_opregion_notify_adapter(dev, PCI_D0);
> -	dev_priv->pm.suspended = false;
> +	if (dev_priv->pm.funcs.runtime_resume)
> +		dev_priv->pm.funcs.runtime_resume(dev_priv);
> +	else
> +		DRM_ERROR("Device does not have runtime functions");
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6a6f046..54aa31d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1322,11 +1322,17 @@ struct i915_package_c8 {
>  	} regsave;
>  };
>  
> +struct i915_runtime_pm_funcs {
> +	int (*runtime_suspend)(struct drm_i915_private *dev_priv);
> +	int (*runtime_resume)(struct drm_i915_private *dev_priv);
> +};
> +
>  struct i915_runtime_pm {
>  	bool suspended;
>  	bool gpu_idle;
>  	bool disp_idle;
>  	struct mutex lock;
> +	struct i915_runtime_pm_funcs funcs;
>  };
>  
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9d6d0e1..6713995 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5588,6 +5588,41 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>  	pm_runtime_put_autosuspend(device);
>  }
>  
> +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	WARN_ON(!HAS_RUNTIME_PM(dev));
> +
> +	i915_gem_release_all_mmaps(dev_priv);
> +
> +	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> +	dev_priv->pm.suspended = true;
> +
> +	/*
> +	 * current versions of firmware which depend on this opregion
> +	 * notification have repurposed the D1 definition to mean
> +	 * "runtime suspended" vs. what you would normally expect (D3)
> +	 * to distinguish it from notifications that might be sent
> +	 * via the suspend path.
> +	 */
> +	intel_opregion_notify_adapter(dev, PCI_D1);
> +
> +
> +	return 0;
> +}
> +
> +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	WARN_ON(!HAS_RUNTIME_PM(dev));
> +
> +	intel_opregion_notify_adapter(dev_priv->dev, PCI_D0);
> +	dev_priv->pm.suspended = false;
> +
> +	return 0;
> +}
> +
>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -5606,6 +5641,9 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>  	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_use_autosuspend(device);
> +
> +	dev_priv->pm.funcs.runtime_suspend = hsw_runtime_suspend;
> +	dev_priv->pm.funcs.runtime_resume = hsw_runtime_resume;
>  }
>  
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
> -- 
> 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