Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 24 Jan 2014 19:23:54 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Fri, 2014-01-24 at 20:35 +0530, Naresh Kumar Kachhi wrote:
> > 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@intel.com wrote:
> > > > > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > > > > 
> > > > > 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@intel.com>
> > > > 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
> 
> 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.

I think Naresh is referring to full D3 state shutdown, where the Gunit
goes away too.  But in that case the GT will be shut down as well, so I
don't think the display vs GT thing by itself is a problem.

-- 
Jesse Barnes, Intel Open Source Technology Center