Re: [Intel-gfx] [PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume

2014-08-14 Thread Imre Deak 
On Wed, 2014-08-13 at 23:07 +0530, sagar.a.kam...@intel.com wrote:
 From: Sagar Kamble sagar.a.kam...@intel.com
 
 With this change, intel_runtime_suspend and intel_runtime_resume functions
 become completely platform agnostic. Platform specific suspend/resume
 changes are moved to intel_suspend_complete and intel_resume_prepare.
 
 Cc: Imre Deak imre.d...@intel.com
 Cc: Paulo Zanoni paulo.r.zan...@intel.com
 Cc: Daniel Vetter daniel.vet...@ffwll.ch
 Cc: Jani Nikula jani.nik...@linux.intel.com
 Cc: Goel, Akash akash.g...@intel.com

For the future: it's not necessary to CC the above people, they all read
the mailing list anyway.

 Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com

Both patches look fine to me:
Reviewed-by: Imre Deak imre.d...@intel.com

 ---
  drivers/gpu/drm/i915/i915_drv.c | 76 
 ++---
  1 file changed, 49 insertions(+), 27 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
 index ec96f9a..88464ad 100644
 --- a/drivers/gpu/drm/i915/i915_drv.c
 +++ b/drivers/gpu/drm/i915/i915_drv.c
 @@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
   return true;
  }
  
 +
 +static int intel_suspend_complete(struct drm_i915_private *dev_priv);
 +static int intel_resume_prepare(struct drm_i915_private *dev_priv);
 +
  static int i915_drm_freeze(struct drm_device *dev)
  {
   struct drm_i915_private *dev_priv = dev-dev_private;
 @@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
   return i915_drm_freeze(drm_dev);
  }
  
 -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
 +static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
  {
   hsw_enable_pc8(dev_priv);
  
   return 0;
  }
  
 -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
 +static int snb_resume_prepare(struct drm_i915_private *dev_priv)
  {
   struct drm_device *dev = dev_priv-dev;
  
 @@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private 
 *dev_priv)
   return 0;
  }
  
 -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
 +static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
  {
   hsw_disable_pc8(dev_priv);
  
 @@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct 
 drm_i915_private *dev_priv)
   I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
  }
  
 -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
 +static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
  {
   u32 mask;
   int err;
 @@ -1335,7 +1339,7 @@ err1:
   return err;
  }
  
 -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
 +static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
  {
   struct drm_device *dev = dev_priv-dev;
   int err;
 @@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device *device)
   cancel_work_sync(dev_priv-rps.work);
   intel_runtime_pm_disable_interrupts(dev);
  
 - if (IS_GEN6(dev)) {
 - ret = 0;
 - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 - ret = hsw_runtime_suspend(dev_priv);
 - } else if (IS_VALLEYVIEW(dev)) {
 - ret = vlv_runtime_suspend(dev_priv);
 - } else {
 - ret = -ENODEV;
 - WARN_ON(1);
 - }
 -
 + ret = intel_suspend_complete(dev_priv);
   if (ret) {
   DRM_ERROR(Runtime suspend failed, disabling it (%d)\n, ret);
   intel_runtime_pm_restore_interrupts(dev);
 @@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device *device)
   intel_opregion_notify_adapter(dev, PCI_D0);
   dev_priv-pm.suspended = false;
  
 - if (IS_GEN6(dev)) {
 - ret = snb_runtime_resume(dev_priv);
 - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 - ret = hsw_runtime_resume(dev_priv);
 - } else if (IS_VALLEYVIEW(dev)) {
 - ret = vlv_runtime_resume(dev_priv);
 - } else {
 - WARN_ON(1);
 - ret = -ENODEV;
 - }
 -
 + ret = intel_resume_prepare(dev_priv);
   /*
* No point of rolling back things in case of an error, as the best
* we can do is to hope that things will still work (and disable RPM).
 @@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device *device)
   return ret;
  }
  
 +static int intel_suspend_complete(struct drm_i915_private *dev_priv)
 +{
 + struct drm_device *dev = dev_priv-dev;
 + int ret;
 +
 + if (IS_GEN6(dev)) {
 + ret = 0;
 + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 + ret = hsw_suspend_complete(dev_priv);
 + } else if (IS_VALLEYVIEW(dev)) {
 + ret = vlv_suspend_complete(dev_priv);
 + } else {
 + ret = -ENODEV;
 + WARN_ON(1);
 + }
 +
 + return ret;
 +}
 +
 +static int intel_resume_prepare(struct drm_i915_private *dev_priv)
 +{
 +

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume

2014-08-14 Thread Daniel Vetter 
On Thu, Aug 14, 2014 at 02:51:15PM +0300, Imre Deak wrote:
 On Wed, 2014-08-13 at 23:07 +0530, sagar.a.kam...@intel.com wrote:
  From: Sagar Kamble sagar.a.kam...@intel.com
  
  With this change, intel_runtime_suspend and intel_runtime_resume functions
  become completely platform agnostic. Platform specific suspend/resume
  changes are moved to intel_suspend_complete and intel_resume_prepare.
  
  Cc: Imre Deak imre.d...@intel.com
  Cc: Paulo Zanoni paulo.r.zan...@intel.com
  Cc: Daniel Vetter daniel.vet...@ffwll.ch
  Cc: Jani Nikula jani.nik...@linux.intel.com
  Cc: Goel, Akash akash.g...@intel.com
 
 For the future: it's not necessary to CC the above people, they all read
 the mailing list anyway.
 
  Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com
 
 Both patches look fine to me:
 Reviewed-by: Imre Deak imre.d...@intel.com

Both merged to dinq (soix for byt sounds like a new feature), thanks.
-Daniel

 
  ---
   drivers/gpu/drm/i915/i915_drv.c | 76 
  ++---
   1 file changed, 49 insertions(+), 27 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_drv.c 
  b/drivers/gpu/drm/i915/i915_drv.c
  index ec96f9a..88464ad 100644
  --- a/drivers/gpu/drm/i915/i915_drv.c
  +++ b/drivers/gpu/drm/i915/i915_drv.c
  @@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
  return true;
   }
   
  +
  +static int intel_suspend_complete(struct drm_i915_private *dev_priv);
  +static int intel_resume_prepare(struct drm_i915_private *dev_priv);
  +
   static int i915_drm_freeze(struct drm_device *dev)
   {
  struct drm_i915_private *dev_priv = dev-dev_private;
  @@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
  return i915_drm_freeze(drm_dev);
   }
   
  -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
  +static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
   {
  hsw_enable_pc8(dev_priv);
   
  return 0;
   }
   
  -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
  +static int snb_resume_prepare(struct drm_i915_private *dev_priv)
   {
  struct drm_device *dev = dev_priv-dev;
   
  @@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private 
  *dev_priv)
  return 0;
   }
   
  -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
  +static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
   {
  hsw_disable_pc8(dev_priv);
   
  @@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct 
  drm_i915_private *dev_priv)
  I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
   }
   
  -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
  +static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
   {
  u32 mask;
  int err;
  @@ -1335,7 +1339,7 @@ err1:
  return err;
   }
   
  -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
  +static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
   {
  struct drm_device *dev = dev_priv-dev;
  int err;
  @@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device 
  *device)
  cancel_work_sync(dev_priv-rps.work);
  intel_runtime_pm_disable_interrupts(dev);
   
  -   if (IS_GEN6(dev)) {
  -   ret = 0;
  -   } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
  -   ret = hsw_runtime_suspend(dev_priv);
  -   } else if (IS_VALLEYVIEW(dev)) {
  -   ret = vlv_runtime_suspend(dev_priv);
  -   } else {
  -   ret = -ENODEV;
  -   WARN_ON(1);
  -   }
  -
  +   ret = intel_suspend_complete(dev_priv);
  if (ret) {
  DRM_ERROR(Runtime suspend failed, disabling it (%d)\n, ret);
  intel_runtime_pm_restore_interrupts(dev);
  @@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device 
  *device)
  intel_opregion_notify_adapter(dev, PCI_D0);
  dev_priv-pm.suspended = false;
   
  -   if (IS_GEN6(dev)) {
  -   ret = snb_runtime_resume(dev_priv);
  -   } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
  -   ret = hsw_runtime_resume(dev_priv);
  -   } else if (IS_VALLEYVIEW(dev)) {
  -   ret = vlv_runtime_resume(dev_priv);
  -   } else {
  -   WARN_ON(1);
  -   ret = -ENODEV;
  -   }
  -
  +   ret = intel_resume_prepare(dev_priv);
  /*
   * No point of rolling back things in case of an error, as the best
   * we can do is to hope that things will still work (and disable RPM).
  @@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device 
  *device)
  return ret;
   }
   
  +static int intel_suspend_complete(struct drm_i915_private *dev_priv)
  +{
  +   struct drm_device *dev = dev_priv-dev;
  +   int ret;
  +
  +   if (IS_GEN6(dev)) {
  +   ret = 0;
  +   } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
  +   ret = hsw_suspend_complete(dev_priv);
  +   } else if (IS_VALLEYVIEW(dev)) {
  +   ret = vlv_suspend_complete(dev_priv);
  +   }