On Wed, Jan 22, 2014 at 05:34:21PM +0530, naresh.kumar.kachhi@xxxxxxxxx wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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@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
