From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [RFC 0/6] Haswell runtime PM support + D3 Date: Mon, 28 Oct 2013 15:09:11 +0200 Message-ID: <1382965751.5775.62.camel@intelbox> References: <1382470214-1597-1-git-send-email-przanoni@gmail.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1859196238==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 6E257E5F44 for ; Mon, 28 Oct 2013 06:09:14 -0700 (PDT) In-Reply-To: <1382470214-1597-1-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org --===============1859196238== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-tiqGd+cq5LO3Dw9Chbgv" --=-tiqGd+cq5LO3Dw9Chbgv Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote: > From: Paulo Zanoni >=20 > Hi >=20 > This RFC series adds runtime PM support on Haswell. The current implement= ation > puts the device in the PCI D3cold state when we decide to sleep. It uses = the > same refcount+timeout idea from the PC8 code, but now through the Kernel = runtime > PM infrastructure. >=20 > I saw Jesse and Imre are still playing with the power wells on Baytrail, = so I > thought maybe they would take some time to do the whole D3 + runtime_pm t= hing, > so I decided to ressurrect some code I had, test it and send anyway. >=20 > The basic idea of this series is that it adds some functions that should = be > reused when we add support for other platforms, but OTOH it completely re= lies on > the already-implemented PC8 support for saving+restoring everything on Ha= swell. > So even though the code is ready for Haswell, it's not ready for !Haswell= . >=20 > So we'll only reach the zero refcount after we enable PC8, and we'll incr= ement > the refcount just before disabling PC8. I thought we would need more regi= ster > save-restore code, but it seems the current save-restore code from PC8 is= enough > to keep everything going. All I needed was to add some more put/get calls= to > keep the device awake when needed. >=20 > Now here's the reason why this is still an RFC: I just added put/get call= s to > the places I spotted problems while doing my basic tests. I basically tes= ted > runtime PM while running Gnome and Xfce, and I also tested sysfs and debu= gfs. I > still didn't write the full test suite I've been promising to write: this= will > be my next step. OTOH, the runtime PM support is disabled by default, so = merging > these patches shouldn't really hurt. >=20 > But our driver has so many entry points that I just don't think we'll eve= r be > confident that we always wake up from D3 when needed, and I don't think w= e'll be > able to write a single test case that tests everything. So I fear this fe= ature > will be just like the "unclaimed register" checking code: we'll be stuck = on a > break-bisect-fix loop forever. And considering most people won't ever run= the > runtime PM code, we may stay long periods of time without noticing the br= oken > state. >=20 > When we add support for BYT and other platforms we will have to rearrange= the > PC8 code so that we move some things out of it to some common PM infrastr= ucture > (i.e, dev_priv->pm). The first things I can think of are the code that co= unts > the current number of used CRTCs and the code that handles pc8.gpu_idle. >=20 > The interfaces for configuring the runtime PM support are the standard ru= ntime > PM interfaces. Go to /sys/bus/pci/devices/0000\:00\:02.0/power/ and have = some > fun. I suggest you to "echo 5 > autosuspend_delay_ms" and boot with > "i915.pc8_timeout=3D5" if you really want to test things. >=20 > Another thing which we could discuss is if we really want to keep PC8 and= D3 as > separate features. Today they're separate features, where D3 is a "deeper= " step > that can be disabled while PC8 is still enabled. This creates the problem= that > we need to separately test/validate PC8-only and PC8+D3 (and possibly D3-= only in > the future, depending on how we rearrange code). Since I can't really thi= nk of a > case where we would want PC8 but not D3, maybe we should kill the "PC8 wi= thout > D3" option. >=20 > Opinions? Bikesheds? Does this really looks like it can be resued on BYT?= Jesse? > Imre? I read through the patchset, in general it looks good to me. I agree it would make sense to merge already now at least the parts that would enable others to sort out platform specific details like HW state/restore for !HSW. I'm also wondering where to place the get/put calls, here is my 2 cents about this, please correct me where I'm wrong: With RPM we want to control whether the whole device is in D0 or D3/D3_cold state, which is the coarsest granularity. While in D0 we also need a finer granularity control to gate as many clocks and power domains as we can (while others remain active preventing D3/D3_cold). For this we enable/disable PC8/RC6 to let the HW do automatic gating, or we do this manually for example through power domains control. If this is correct we should try our best to only do fine grain control at the highest level and do RPM get/put only as a consequence. For example modeset code would only enable/disable whatever power domains are needed for the current mode and the power domain framework would do the RPM get/put. Your PC8 changes in your patchset are in accordance with this: at the highest level we only enable/disable PC8 which will do an RPM get/put if needed. Debugfs and other places where we need register access: atm you do only an RPM get/put around these, but this just ensures we are in D0, not that we have the needed power domains/clocks enabled. I guess this works for HSW, but it may not work on other platforms. One solution could be to do the same here as for modeset, we just enable/disable clocks and power domains around these places as needed and adjust the RPM refcount only as a consequence. I also have a few nitpicks about the patches, sending those inlined. --Imre --=-tiqGd+cq5LO3Dw9Chbgv 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.12 (GNU/Linux) iQEcBAABAgAGBQJSbmH3AAoJEORIIAnNuWDFFsQIALwutP5irwUrI8wfGxH9JjKm 9oV7sQ+Yq/KtfqsW3M/zAAvGtoea4xMTq1e+3aY3NK2xG0UwR5TSW4J2h7dNgbrk jlovdIoygcfv8nuQ/Aag61JV+TFkjS+lhILY8MukOfOnQoZCNU4wh2hkhyEZ/B+R SWWpsQmFLevYmBMRPeYoVOQeIu0xaL9eSoLvK/3QbMrkHlt1onMyIL9JpsW4UIQ/ IgNpI8u6hD8vlEWOwaV60M2gS6SDavwmfTNo7jl4bdFkbWKXG4zFiTgfklyHTWAB 8VVQ+x3lxkksoo1pkh8W1mb7QQJtPAAGAn+eAJ8Exxe7t61TF2HocOGhwLi4nXU= =Mt66 -----END PGP SIGNATURE----- --=-tiqGd+cq5LO3Dw9Chbgv-- --===============1859196238== 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 --===============1859196238==--