From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put Date: Fri, 24 Jan 2014 19:23:54 +0200 Message-ID: <1390584234.9428.20.camel@intelbox> References: <1390392262-30937-1-git-send-email-naresh.kumar.kachhi@intel.com> <1390392262-30937-2-git-send-email-naresh.kumar.kachhi@intel.com> <20140122125122.GG9772@phenom.ffwll.local> <1390397008.12277.15.camel@intelbox> <52E2814B.4030305@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0828367667==" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id AB95DFA555 for ; Fri, 24 Jan 2014 09:24:33 -0800 (PST) In-Reply-To: <52E2814B.4030305@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Naresh Kumar Kachhi Cc: intel-gfx@lists.freedesktop.org, "Satyanantha, Rama Gopal M" List-Id: intel-gfx@lists.freedesktop.org --===============0828367667== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-cS0cdrFegdE7tFjFFgfW" --=-cS0cdrFegdE7tFjFFgfW Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2014-01-24 at 20:35 +0530, Naresh Kumar Kachhi wrote: > On 01/22/2014 06:53 PM, Imre Deak wrote: >=20 > > On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote: > > > On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.c= om wrote: > > > > From: Naresh Kumar Kachhi > > > >=20 > > > > With runtime PM enabled, we need to make sure that all HW access > > > > are valid (i.e. Gfx is in D0). Invalid accesses might end up in > > > > HW hangs. Ex. A hang is seen if display register is accessed on > > > > BYT while display power island is power gated. > > > >=20 > > > > This patch is covering all the IOCTLs with get/put. > > > > TODO: limit runtime_get/put to IOCTLs that accesses HW > > > >=20 > > > > Signed-off-by: Naresh Kumar Kachhi > > > Nack on the concept. We need to have get/put calls for the individual > > > functional blocks of the hw, which then in turn (if it's not the top-= level > > > power domain) need to grab references to the next up power domain. > > > Splattering unconditional get/puts over all driver entry points is ba= d > > > style and imo also too fragile. > > >=20 > > > Also, with Paulos final runtime pm/pc8 unification patches and Imre's > > > display power well patches for byt we should have full coverage alrea= dy. > > > Have you looked at the work of these too? > > I'm still in debt with the BYT specific power domain patches, I want to > > post them (this week) after I sorted out spurious pipe stat IRQs we'd > > receive atm with the power well off. Until then there is only the WIP > > version at: https://github.com/ideak/linux/commits/powerwells > >=20 > > But in practice, as you point out the plan was to only call > > modeset_update_power_wells() during modeset time and that will end up > > doing the proper RPM get/put as necessary. Similarly on some other code > > paths like connector detect and HW state read-out code, we'd ask only > > for the needed power domains which would end up doing the RPM get/put. > >=20 > > --Imre > >=20 > Hi Imre, > I tried to go through your and Paulo's patches (http://patchwork.freedesk= top.org/patch/16952/). > As per my understanding we are doing disp power well gate/ungate independ= ent of we are in=20 > runtime_suspend/resume state we might face problem here as on BYT interru= pts are routed through > display power well, so we might have a situation where display power isla= nd is power gated, > but render workload is still active. As we won't be receiving any interru= pt __wait_seq_no will > stall and we might end up in a TDR. We will not see the issue until displ= ay power gating is=20 > enabled. I will try to include a test to cover this test Hm, which exact interrupt routing are you referring to? At least on my BYT I manage to power off only the display power well and keep the render well on, while still being able to run for example igt/tests/gem_render_copy, gem_render_linear_blits. I can see that through /proc/interrupts that GT interrupts are properly generated. The display side interrupt routing is of course off, for example the PIPESTAT registers will read all 0xffffffff, but DEIIR, DEIMR, DEISR are all seem to work ok. --Imre > This can be avoided by adding display_get/put in runtime_resume/suspend. > I am not sure if this scenario will be applicable for HSW. >=20 > Need for covering ioclts: > However there is one more problem for LP platforms. Once display/render/m= edia power > islands are power gated, system will enter into deeper sleep state causin= g Gunit to power > gate. We will loose the state once Gunit resumes, so Gunit restore and ri= ng initialization is > required in runtime_resume. Even after adding these to runtime_suspend/re= sume, we will have > to make sure all the ring activities, Gunit register accesses are covered= with runtime_get/put. > There are few places with current implementation where we might hit this = problem. > Example:=20 > i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled) > i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled) > i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu co= mmands scheduled) > There are few more. > These ioclts not being covered with runtime_get/put will might cause inva= lid executions > if we are resuming for deeper power state (gunit was powergated). >=20 > solutions: > From design perspective, we can cover all accesses at low level, but this= means a lot of get/put > and also will have to make sure that we cover future accesses as well. As= an other solution we > can cover these ioctls individually or can have a wrapper function (as pu= rposed in this patch) > to cover the accesses (TODO: make them conditional to do get/put on few i= oclts only). > I will try to upload refined patch with conditional get/put. >=20 > Thanks, > Naresh >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 --=-cS0cdrFegdE7tFjFFgfW 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) iQEcBAABAgAGBQJS4qGqAAoJEORIIAnNuWDFEPUIAI+un9whcalEA/uUqdLgrLj6 C3/Kr/9GyZBnhDnsfa0cJf/uPkKBTD/isDoUy7b+JdiG9Mr+f9tab8/sQ1UjNMQ8 O38t0Di8CbjIb/P2pKCmcanaLi9J4nOsQLGc6DtMvWreEEws115A+Hnb5pidKLd7 /WX8loP44V1mMxbFuiKK7/VWOGbbJpiZkhuedp+fRHMqB4EYx+Dh2PPwSmBFRtBC L63R01Ny/ul1sqr5Lxw3XqmqP0oabS5B+uAX3PeVfnN4Vq6NXNKsSz7W7Sk2d+6r 4ha5CSAzYFsRTFyAqOWTYBJMuDGLCAJXLlZdHekwrAu6nBFWGCwcowKjoQtXj/U= =ts5N -----END PGP SIGNATURE----- --=-cS0cdrFegdE7tFjFFgfW-- --===============0828367667== 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 --===============0828367667==--