<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 01/22/2014 06:53 PM, Imre Deak
wrote:<br>
</div>
<blockquote cite="mid:1390397008.12277.15.camel@intelbox"
type="cite">
<pre wrap="">On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Wed, Jan 22, 2014 at 05:34:17PM +0530, <a class="moz-txt-link-abbreviated" href="mailto:naresh.kumar.kachhi@intel.com">naresh.kumar.kachhi@intel.com</a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">From: Naresh Kumar Kachhi <a class="moz-txt-link-rfc2396E" href="mailto:naresh.kumar.kachhi@intel.com"><naresh.kumar.kachhi@intel.com></a>
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 <a class="moz-txt-link-rfc2396E" href="mailto:naresh.kumar.kachhi@intel.com"><naresh.kumar.kachhi@intel.com></a>
</pre>
</blockquote>
<pre wrap="">
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?
</pre>
</blockquote>
<pre wrap="">
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: <a class="moz-txt-link-freetext" href="https://github.com/ideak/linux/commits/powerwells">https://github.com/ideak/linux/commits/powerwells</a>
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
</pre>
</blockquote>
<pre>Hi Imre,
I tried to go through your and Paulo's patches (<a class="moz-txt-link-freetext" href="http://patchwork.freedesktop.org/patch/16952/">http://patchwork.freedesktop.org/patch/16952/</a>).
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
<meta http-equiv="content-type" content="text/html; charset=UTF-8">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).
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
</pre>
</body>
</html>