subject:"\[Intel\-gfx\] \[PATCH v3 1\/1\] drm\/i915\: Sharing Gfx Clock, Wake and Gunit save\/restore logic using common handler for runtime\/system s\/r paths"

Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

2014-08-13 Thread Imre Deak

On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kam...@intel.com wrote:
 From: Sagar Kamble sagar.a.kamble at intel.com

The change is substantial enough that you should add a commit message
explaining what it fixes and how. There are further useful guidelines on
this topic in Documentation/SubmittingPatches.

In the future, you would make the review (and a possible bisection)
easier if you split this patch into a refactoring patch without
functional change and one that fixes the issue.

 v1:
 Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of 
 gunit registers need to be followed in
 PM suspend and resume path similar to runtime suspend and resume.
 
 v2:
 1. Keeping GT access, wake, gunit save/restore related helpers static.
 2. Moved GT access check, Wake Control, Gunit state save to end of 
 i915_drm_freeze.
 3. Reusing the sequence in runtime_suspend/resume path at macro level.
 
 v3:
 1. Prepared common handlers for platform specific tasks to be done before HW 
 suspend and after HW resume from
 D0i3.
 2. Changed commit header.
 
 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
 Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
 Signed-off-by: Sagar Kamble sagar.a.kamble at intel.com
 ---
  drivers/gpu/drm/i915/i915_drv.c | 130 
 ++--
  1 file changed, 85 insertions(+), 45 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
 index ec96f9a..4440722 100644
 --- a/drivers/gpu/drm/i915/i915_drv.c
 +++ b/drivers/gpu/drm/i915/i915_drv.c
 @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
   return true;
  }
  
 +
 +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
 +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
 + bool resume_from_rpm_suspend);

Nitpick: Something like intel_suspend_complete, intel_resume_prepare
would be more descriptive names.

 +
  static int i915_drm_freeze(struct drm_device *dev)
  {
   struct drm_i915_private *dev_priv = dev-dev_private;
 @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
  static int i915_drm_thaw_early(struct drm_device *dev)
  {
   struct drm_i915_private *dev_priv = dev-dev_private;
 + int ret = 0;

The initialization here is redundant and could suppress complier
warnings. There are also a couple more instances below.
 
 - if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 - hsw_disable_pc8(dev_priv);
 + /* Restore any platform specific registers/clk state */
 + ret = post_hw_resume_init(dev_priv, false);

You could print an error message here, noting that we continue resuming
despite the error.

   intel_uncore_early_sanitize(dev, true);
   intel_uncore_sanitize(dev);
   intel_power_domains_init_hw(dev_priv);
  
 - return 0;
 + return ret;
  }
  
  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
   struct pci_dev *pdev = to_pci_dev(dev);
   struct drm_device *drm_dev = pci_get_drvdata(pdev);
   struct drm_i915_private *dev_priv = drm_dev-dev_private;
 + int ret = 0;
  
   /*
* We have a suspedn ordering issue with the snd-hda driver also
 @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
   if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
   return 0;
  
 - if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
 - hsw_enable_pc8(dev_priv);
 + /* Save any platform specific registers/clk state needed post resume */
 + ret = pre_hw_suspend_deinit(dev_priv);
  
   pci_disable_device(pdev);
   pci_set_power_state(pdev, PCI_D3hot);

In case of error the suspend will be canceled and the resume handler
will be called, so we shouldn't disable the device.

  
 - return 0;
 + return ret;
  }
  
  static int i915_pm_resume_early(struct device *dev)
 @@ -983,23 +990,26 @@ 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(struct drm_i915_private *dev_priv)

Based on the above nitpick something like hsw_suspend_prepare would be
better. The same goes for the other platform handlers.

  {
   hsw_enable_pc8(dev_priv);
  
   return 0;
  }
  
 -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
 +static int snb_resume(struct drm_i915_private *dev_priv,
 + bool resume_from_rpm_suspend)

Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more
compact.

  {
   struct drm_device *dev = dev_priv-dev;
  
 - intel_init_pch_refclk(dev);
 + if

Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

2014-08-13 Thread Sagar Arun Kamble

On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote:
 On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kam...@intel.com wrote:
  From: Sagar Kamble sagar.a.kamble at intel.com
 
 The change is substantial enough that you should add a commit message
 explaining what it fixes and how. There are further useful guidelines on
 this topic in Documentation/SubmittingPatches.
 
 In the future, you would make the review (and a possible bisection)
 easier if you split this patch into a refactoring patch without
 functional change and one that fixes the issue.
Thanks for the review Imre, Daniel.
Agree that I need to split this patch and give more description for the
changes. Will fix other reviewed code as well.
 
  v1:
  Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of 
  gunit registers need to be followed in
  PM suspend and resume path similar to runtime suspend and resume.
  
  v2:
  1. Keeping GT access, wake, gunit save/restore related helpers static.
  2. Moved GT access check, Wake Control, Gunit state save to end of 
  i915_drm_freeze.
  3. Reusing the sequence in runtime_suspend/resume path at macro level.
  
  v3:
  1. Prepared common handlers for platform specific tasks to be done before 
  HW suspend and after HW resume from
  D0i3.
  2. Changed commit header.
  
  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
  Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
  Signed-off-by: Sagar Kamble sagar.a.kamble at intel.com
  ---
   drivers/gpu/drm/i915/i915_drv.c | 130 
  ++--
   1 file changed, 85 insertions(+), 45 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_drv.c 
  b/drivers/gpu/drm/i915/i915_drv.c
  index ec96f9a..4440722 100644
  --- a/drivers/gpu/drm/i915/i915_drv.c
  +++ b/drivers/gpu/drm/i915/i915_drv.c
  @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
  return true;
   }
   
  +
  +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
  +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
  +   bool resume_from_rpm_suspend);
 
 Nitpick: Something like intel_suspend_complete, intel_resume_prepare
 would be more descriptive names.
 
  +
   static int i915_drm_freeze(struct drm_device *dev)
   {
  struct drm_i915_private *dev_priv = dev-dev_private;
  @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
   static int i915_drm_thaw_early(struct drm_device *dev)
   {
  struct drm_i915_private *dev_priv = dev-dev_private;
  +   int ret = 0;
 
 The initialization here is redundant and could suppress complier
 warnings. There are also a couple more instances below.
  
  -   if (IS_HASWELL(dev) || IS_BROADWELL(dev))
  -   hsw_disable_pc8(dev_priv);
  +   /* Restore any platform specific registers/clk state */
  +   ret = post_hw_resume_init(dev_priv, false);
 
 You could print an error message here, noting that we continue resuming
 despite the error.
 
  intel_uncore_early_sanitize(dev, true);
  intel_uncore_sanitize(dev);
  intel_power_domains_init_hw(dev_priv);
   
  -   return 0;
  +   return ret;
   }
   
   static int __i915_drm_thaw(struct drm_device *dev, bool 
  restore_gtt_mappings)
  @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
  struct pci_dev *pdev = to_pci_dev(dev);
  struct drm_device *drm_dev = pci_get_drvdata(pdev);
  struct drm_i915_private *dev_priv = drm_dev-dev_private;
  +   int ret = 0;
   
  /*
   * We have a suspedn ordering issue with the snd-hda driver also
  @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
  if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
  return 0;
   
  -   if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
  -   hsw_enable_pc8(dev_priv);
  +   /* Save any platform specific registers/clk state needed post resume */
  +   ret = pre_hw_suspend_deinit(dev_priv);
   
  pci_disable_device(pdev);
  pci_set_power_state(pdev, PCI_D3hot);
 
 In case of error the suspend will be canceled and the resume handler
 will be called, so we shouldn't disable the device.
 
   
  -   return 0;
  +   return ret;
   }
   
   static int i915_pm_resume_early(struct device *dev)
  @@ -983,23 +990,26 @@ 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(struct drm_i915_private *dev_priv)
 
 Based on the above nitpick something like hsw_suspend_prepare would be
 better. The same goes for the other platform handlers.
 
   {
  hsw_enable_pc8(dev_priv);
   
  return 0;
   }
   
  -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
  +static int snb_resume(struct

[Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

2014-08-12 Thread sagar . a . kamble

From: Sagar Kamble sagar.a.kamble at intel.com

v1:
Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of 
gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.

v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of 
i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.

v3:
1. Prepared common handlers for platform specific tasks to be done before HW 
suspend and after HW resume from
D0i3.
2. Changed commit header.

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
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble sagar.a.kamble at intel.com
---
 drivers/gpu/drm/i915/i915_drv.c | 130 ++--
 1 file changed, 85 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..4440722 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
return true;
 }
 
+
+static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
+static int post_hw_resume_init(struct drm_i915_private *dev_priv,
+   bool resume_from_rpm_suspend);
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
+   int ret = 0;
 
-   if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-   hsw_disable_pc8(dev_priv);
+   /* Restore any platform specific registers/clk state */
+   ret = post_hw_resume_init(dev_priv, false);
 
intel_uncore_early_sanitize(dev, true);
intel_uncore_sanitize(dev);
intel_power_domains_init_hw(dev_priv);
 
-   return 0;
+   return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct drm_i915_private *dev_priv = drm_dev-dev_private;
+   int ret = 0;
 
/*
 * We have a suspedn ordering issue with the snd-hda driver also
@@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
 
-   if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
-   hsw_enable_pc8(dev_priv);
+   /* Save any platform specific registers/clk state needed post resume */
+   ret = pre_hw_suspend_deinit(dev_priv);
 
pci_disable_device(pdev);
pci_set_power_state(pdev, PCI_D3hot);
 
-   return 0;
+   return ret;
 }
 
 static int i915_pm_resume_early(struct device *dev)
@@ -983,23 +990,26 @@ 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(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(struct drm_i915_private *dev_priv,
+   bool resume_from_rpm_suspend)
 {
struct drm_device *dev = dev_priv-dev;
 
-   intel_init_pch_refclk(dev);
+   if (resume_from_rpm_suspend)
+   intel_init_pch_refclk(dev);
 
return 0;
 }
 
-static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
+static int hsw_resume(struct drm_i915_private *dev_priv,
+   bool resume_from_rpm_suspend)
 {
hsw_disable_pc8(dev_priv);
 
@@ -1295,10 +1305,10 @@ 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(struct drm_i915_private *dev_priv)
 {
u32 mask;
-   int err;
+   int ret = 0;
 
/*
 * Bspec defines the following GT well on flags as debug only, so
@@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private 
*dev_priv)
 
vlv_check_no_gt_access(dev_priv);
 
-   err = vlv_force_gfx_clock(dev_priv, true);
-   if (err)
+   ret = vlv_force_gfx_clock(dev_priv, true);
+   if (ret)
goto err1;
 
-   err = vlv_allow_gt_wake(dev_priv, false);
-   if (err)
+   ret

Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

3 matches

Site Navigation

Mail list logo

Footer information