From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths Date: Wed, 13 Aug 2014 16:47:10 +0300 Message-ID: <1407937630.27091.42.camel@intelbox> References: <20140808140126.GT8727@phenom.ffwll.local> <1407840731-25081-1-git-send-email-sagar.a.kamble@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1874236678==" Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id D30136E093 for ; Wed, 13 Aug 2014 06:47:13 -0700 (PDT) In-Reply-To: <1407840731-25081-1-git-send-email-sagar.a.kamble@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: sagar.a.kamble@intel.com Cc: "Goel, Akash" , Daniel Vetter , intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org --===============1874236678== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-yX1C6OFBAalV25nOxdjV" --=-yX1C6OFBAalV25nOxdjV Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble 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 o= f gunit registers need to be followed in > PM suspend and resume path similar to runtime suspend and resume. >=20 > 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_d= rm_freeze. > 3. Reusing the sequence in runtime_suspend/resume path at macro level. >=20 > 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. >=20 > Cc: Imre Deak > Cc: Paulo Zanoni > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Goel, Akash > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > Signed-off-by: Sagar Kamble > --- > drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------= ------ > 1 file changed, 85 insertions(+), 45 deletions(-) >=20 > 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 *de= v) > return true; > } > =20 > + > +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 =3D 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 =3D dev->dev_private; > + int ret =3D 0; The initialization here is redundant and could suppress complier warnings. There are also a couple more instances below. =20 > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > - hsw_disable_pc8(dev_priv); > + /* Restore any platform specific registers/clk state */ > + ret =3D 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); > =20 > - return 0; > + return ret; > } > =20 > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mapp= ings) > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev) > struct pci_dev *pdev =3D to_pci_dev(dev); > struct drm_device *drm_dev =3D pci_get_drvdata(pdev); > struct drm_i915_private *dev_priv =3D drm_dev->dev_private; > + int ret =3D 0; > =20 > /* > * 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 =3D=3D DRM_SWITCH_POWER_OFF) > return 0; > =20 > - 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 =3D pre_hw_suspend_deinit(dev_priv); > =20 > 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. > =20 > - return 0; > + return ret; > } > =20 > 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); > } > =20 > -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); > =20 > return 0; > } > =20 > -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 =3D dev_priv->dev; > =20 > - intel_init_pch_refclk(dev); > + if (resume_from_rpm_suspend) > + intel_init_pch_refclk(dev); > =20 > return 0; > } > =20 > -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); > =20 > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i91= 5_private *dev_priv) > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > } > =20 > -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 =3D 0; > =20 > /* > * Bspec defines the following GT well on flags as debug only, so > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_pr= ivate *dev_priv) > =20 > vlv_check_no_gt_access(dev_priv); > =20 > - err =3D vlv_force_gfx_clock(dev_priv, true); > - if (err) > + ret =3D vlv_force_gfx_clock(dev_priv, true); > + if (ret) > goto err1; > =20 > - err =3D vlv_allow_gt_wake(dev_priv, false); > - if (err) > + ret =3D vlv_allow_gt_wake(dev_priv, false); > + if (ret) > goto err2; > vlv_save_gunit_s0ix_state(dev_priv); > =20 > - err =3D vlv_force_gfx_clock(dev_priv, false); > - if (err) > + ret =3D vlv_force_gfx_clock(dev_priv, false); > + if (ret) > goto err2; > - > - return 0; > + return ret; > =20 > err2: > /* For safety always re-enable waking and disable gfx clock forcing */ > @@ -1332,14 +1341,15 @@ err2: > err1: > vlv_force_gfx_clock(dev_priv, false); > =20 > - return err; > + return ret; > } In the above function I can't see any change besides renaming err to ret. There isn't much point in that imo. > =20 > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > +static int vlv_resume(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > { > struct drm_device *dev =3D dev_priv->dev; > int err; > - int ret; > + int ret =3D 0; > =20 > /* > * If any of the steps fail just try to continue, that's the best we > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_priv= ate *dev_priv) > =20 > vlv_check_no_gt_access(dev_priv); > =20 > - intel_init_clock_gating(dev); > - i915_gem_restore_fences(dev); > + if (resume_from_rpm_suspend) { > + intel_init_clock_gating(dev); > + i915_gem_restore_fences(dev); > + } > =20 > return ret; > } > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *de= vice) > cancel_work_sync(&dev_priv->rps.work); > intel_runtime_pm_disable_interrupts(dev); > =20 > - if (IS_GEN6(dev)) { > - ret =3D 0; > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret =3D hsw_runtime_suspend(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret =3D vlv_runtime_suspend(dev_priv); > - } else { > - ret =3D -ENODEV; > - WARN_ON(1); > - } > + /* Save any platform specific registers/clk state needed post resume */ > + ret =3D pre_hw_suspend_deinit(dev_priv); > =20 > if (ret) { > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *dev= ice) > intel_opregion_notify_adapter(dev, PCI_D0); > dev_priv->pm.suspended =3D false; > =20 > - if (IS_GEN6(dev)) { > - ret =3D snb_runtime_resume(dev_priv); > - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > - ret =3D hsw_runtime_resume(dev_priv); > - } else if (IS_VALLEYVIEW(dev)) { > - ret =3D vlv_runtime_resume(dev_priv); > - } else { > - WARN_ON(1); > - ret =3D -ENODEV; > - } > + /* Restore any platform specific registers/clk state */ > + ret =3D post_hw_resume_init(dev_priv, true); > =20 > /* > * No point of rolling back things in case of an error, as the best > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *dev= ice) > return ret; > } > =20 > +/* This handler is used in system/runtime suspend path to reuse > + * Gfx clock, Wake control, Gunit state save related functionaility for = VLV. > + */ The above comment is not precise, the function is used for all platforms. I think it'd be enough to mention that it's the common part of the runtime and system suspend sequence. > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + int ret =3D 0; > + > + if (IS_GEN6(dev)) { > + ret =3D 0; > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret =3D hsw_suspend(dev_priv); > + } else if (IS_VALLEYVIEW(dev)) { > + ret =3D vlv_suspend(dev_priv); > + } else { > + ret =3D -ENODEV; > + WARN_ON(1); > + } > + > + return ret; > +} > + > +/* This handler is used in system/runtime resume path to reuse > + * Gfx clock, Wake control, Gunit state restore related functionaility f= or VLV. > + */ > +static int post_hw_resume_init(struct drm_i915_private *dev_priv, > + bool resume_from_rpm_suspend) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + int ret =3D 0; > + > + if (IS_GEN6(dev)) { > + ret =3D snb_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > + ret =3D hsw_resume(dev_priv, resume_from_rpm_suspend); > + } else if (IS_VALLEYVIEW(dev)) { > + ret =3D vlv_resume(dev_priv, resume_from_rpm_suspend); > + } else { > + WARN_ON(1); > + ret =3D -ENODEV; > + } > + return ret; > +} > + > static const struct dev_pm_ops i915_pm_ops =3D { > .suspend =3D i915_pm_suspend, > .suspend_late =3D i915_pm_suspend_late, --=-yX1C6OFBAalV25nOxdjV Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJT62xeAAoJEORIIAnNuWDFYmYH/RV5F102lu/zS4nZiPshJwR0 X83je6tQ1ZNi22AUqHrv0jE6nSPItk+8L4ATt+1SS6+MjG7Izkc5V5sxnCGoUukn 8MePhmA3P+pdntthMZBX28uQz4L7HGllf/HoQ+AVReq2wQcFO4w5hZu/NpLcsppE 6GiRbJ1L0DmZIhVDyrECodiD7w9b5mdHFHlx+h6gH2MrbuerRB2OhiXOBhlgzyHR SGBOgl/vTNBlty9uAlcO61rI1eQAVyeu2VXYvdIuQA2G/2y/nYVySO049I80iBcC DwJkqLScohi2wH3uaBMWnpoJZrYmCjKMC2FWb0VlokUOFpByFMry8kE5GQK/Bww= =PbCf -----END PGP SIGNATURE----- --=-yX1C6OFBAalV25nOxdjV-- --===============1874236678== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1874236678==--