From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC 5/6] drm/i915: device specific runtime suspend/resume Date: Wed, 22 Jan 2014 13:58:05 +0100 Message-ID: <20140122125805.GH9772@phenom.ffwll.local> References: <1390392262-30937-1-git-send-email-naresh.kumar.kachhi@intel.com> <1390392262-30937-6-git-send-email-naresh.kumar.kachhi@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id D5FA0FB9BE for ; Wed, 22 Jan 2014 04:58:09 -0800 (PST) Received: by mail-ee0-f44.google.com with SMTP id c13so4842929eek.17 for ; Wed, 22 Jan 2014 04:58:09 -0800 (PST) Content-Disposition: inline In-Reply-To: <1390392262-30937-6-git-send-email-naresh.kumar.kachhi@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: naresh.kumar.kachhi@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Jan 22, 2014 at 05:34:21PM +0530, naresh.kumar.kachhi@intel.com wrote: > From: Naresh Kumar Kachhi > > 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 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@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