2014/1/24 Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx>: > On 01/22/2014 06:53 PM, Imre Deak wrote: > > 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@xxxxxxxxx > wrote: > > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx> > > 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. > > This patch is covering all the IOCTLs with get/put. > TODO: limit runtime_get/put to IOCTLs that accesses HW > > Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx> > > 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 bad > style and imo also too fragile. > > Also, with Paulos final runtime pm/pc8 unification patches and Imre's > display power well patches for byt we should have full coverage already. > 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 > > 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. > > --Imre > > Hi Imre, > I tried to go through your and Paulo's patches > (http://patchwork.freedesktop.org/patch/16952/). > As per my understanding we are doing disp power well gate/ungate independent > of we are in > runtime_suspend/resume state we might face problem here as on BYT interrupts > are routed through > display power well, so we might have a situation where display power island > is power gated, > but render workload is still active. As we won't be receiving any interrupt > __wait_seq_no will > stall and we might end up in a TDR. We will not see the issue until display > power gating is > enabled. I will try to include a test to cover this test > > 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. > > Need for covering ioclts: > However there is one more problem for LP platforms. Once > display/render/media power > islands are power gated, system will enter into deeper sleep state causing > Gunit to power > gate. We will loose the state once Gunit resumes, so Gunit restore and ring > initialization is > required in runtime_resume. Even after adding these to > runtime_suspend/resume, 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: > 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 > commands scheduled) > There are few more. > These ioclts not being covered with runtime_get/put will might cause invalid > executions > if we are resuming for deeper power state (gunit was powergated). If possible, please write pm_pc8.c subtests that reproduce all these problems you found :) > > 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 > purposed in this patch) > to cover the accesses (TODO: make them conditional to do get/put on few > ioclts only). > I will try to upload refined patch with conditional get/put. > > Thanks, > Naresh > > > > > > > > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
