From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4 Date: Thu, 12 Jun 2014 18:08:56 +0300 Message-ID: <1402585736.2480.40.camel@intelbox> References: <1401910389-14722-1-git-send-email-jbarnes@virtuousgeek.org> <1401914722-17532-1-git-send-email-jbarnes@virtuousgeek.org> <1402407770.7876.72.camel@intelbox> <20140610135757.GN5821@phenom.ffwll.local> <1402411309.7876.102.camel@intelbox> <20140610152645.GZ5821@phenom.ffwll.local> <20140611152116.20a3dd30@jbarnes-desktop> <20140611152450.3d4d6c5c@jbarnes-desktop> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1161327689==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id A965B6E928 for ; Thu, 12 Jun 2014 08:09:52 -0700 (PDT) In-Reply-To: <20140611152450.3d4d6c5c@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1161327689== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-s5BOAQfoGoeqBW0KX6JF" --=-s5BOAQfoGoeqBW0KX6JF Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-06-11 at 15:24 -0700, Jesse Barnes wrote: > On Wed, 11 Jun 2014 15:21:16 -0700 > Jesse Barnes wrote: >=20 > > On Tue, 10 Jun 2014 17:26:45 +0200 > > Daniel Vetter wrote: > >=20 > > > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote: > > > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > > > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > > > > > This allows the system to enter the lowest power mode during = system freeze. > > > > > > >=20 > > > > > > > v2: delete force wake timer at suspend (Imre) > > > > > > > v3: add GT work suspend function (Imre) > > > > > > > v4: use uncore forcewake reset (Daniel) > > > > > > >=20 > > > > > > > Signed-off-by: Kristen Carlson Accardi > > > > > > > Signed-off-by: Jesse Barnes > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 20 +++++++++++++++++++= + > > > > > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > >=20 > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/dr= m/i915/i915_drv.c > > > > > > > index 66c6ffb..7148eac 100644 > > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_dev= ice *dev) > > > > > > > drm_irq_uninstall(dev); > > > > > > > dev_priv->enable_hotplug_processing =3D false; > > > > > > > =20 > > > > > > > - intel_disable_gt_powersave(dev); > > > > > > > + intel_suspend_gt_powersave(dev); > > > > > >=20 > > > > > > I realized now that we actually do need to enable RC6 explicitl= y. If we > > > > > > get here right after runtime resume, the deferred RC6 enabling = might be > > > > > > still pending and so RC6 might not be enabled. > >=20 > > Seems like we could just flush the enable if it was outstanding? >=20 > Oh and I see we already have the flush in v4... Right, I was just blind and I guess also thinking in terms of timers that don't have a similar flush API. Anyway this is fine and my comment about it above can be ignored. > do you think we might not actually arm the deferred enable work before > we get here?=20 I haven't thought about this. But now that you mentioned I looked into the device and dpm core and can't see what would prevent our system suspend handler to be called while we are still running our driver load function. But this is a separate issue (if true at all) that exists regardless of this patch and also not a really real life scenario. So I suggest we investigate this as a follow-up. > I think the runtime_resume and suspend calls should be serialized, but > if not we'd need some locking I guess... Yes, this is taken care of the RPM framework. Similarly system suspend/resume handlers are serialized wrt. the runtime suspend/resume handlers. This patch is: Reviewed-by: Imre Deak Btw, now all 4 patches in the patchset are reviewed. --=-s5BOAQfoGoeqBW0KX6JF 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) iQEcBAABAgAGBQJTmcKIAAoJEORIIAnNuWDFMmwIAL1EVOusSGTpPbejkbImL74G IuM2PGa6vHEv212+cQ4n8L1oi4HWth3uCvCnOvBLFqnxZvQDsPjBuDC8I9xQypdP 3WDm8gK+6H+4Ckf0KwV6LWsnTG9/ciU9xP4pgJGm3LO5sOY4WceyLk9AtRw8fHsF D/tv4QeDXPZWghTzc1FaVcEhKUxWuYFK9RE9m3MBh3SHWfXghD161ALJVLVN/0fa AAyAJYro5etRIEPR4mwX9CsbIQDH8seySgDwDHVj5MQsgQxC+D8petXgRvn4jbw0 FD48OasY6L68nXFZ0brxjK45Uwxiqqe9ZhJsk1ezD/ylJENmMR2zz/AUxoaJmPE= =FF4Z -----END PGP SIGNATURE----- --=-s5BOAQfoGoeqBW0KX6JF-- --===============1161327689== 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 --===============1161327689==--