Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 31, 2014 at 03:22:36PM -0300, Paulo Zanoni wrote:
> 2014-03-14 15:43 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote:
> >>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
> >>> > ---
> >>> >  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
> >>> >  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
> >>> >  2 files changed, 3 insertions(+), 8 deletions(-)
> >>> >
> >>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>> > index 5a0d34c47885..3fbf8aa8d119 100644
> >>> > --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
> >>> >     struct drm_i915_private *dev_priv = dev->dev_private;
> >>> >
> >>> >     WARN_ON(!HAS_RUNTIME_PM(dev));
> >>> > -   assert_force_wake_inactive(dev_priv);
> >>>
> >>> Why is this necessary? Also I've already pushed a pile of other patches on
> >>> top of all this, so I think a full commit is better. Also gives us an
> >>> excuse to document our flailing here a bit better in a neat commit message
> >>> ... Imo we should also mention that the forcewake_put here isn't really
> >>> perf critical any more (if this is really the case).
> >>
> >> I was continuing the conversation with example code... This is, I think,
> >> the simplest method for removing the pm_put from the forcewake timer,
> >> and just wanted to make sure that we were in agreement before writing a
> >> paragraph to explain the problem.
> >
> > Ah, with closer reading of your patch I've noticed that the
> > uncore_fini is after the above assert, so this indeed has to go.
> >
> > I'm also ok with the overall patch if that doesn't cause another round
> > back to reinstate the delayed forcewake put here ;-)
> 
> Hi
> 
> Last week Chris sent me a rebased version of this patch on pastebin. I
> tested it, and when I run the "rte" subtest from pm_pc8, I get many
> instances of the "WARN_ON(dev->irq_enabled)" that happens inside
> intel_disable_gt_powersave().
> 
> I also tried to apply "drm/i915: Fix runtime PM inbalance due to reg
> I/O forcewake dance", but the patch does not apply cleanly.
> 
> That leaves us with the original "drm/i915: don't schedule
> force_wake_timer at gen6_read".
> 
> I'd really like to get this bug fixed ASAP as it completely prevents
> runtime PM from working at all, and we already have fixes for it since
> weeks ago. Daniel? Chris?

Agreed that the your patch which essentially reverts stuff is the best
course for getting 3.15 into shape. We can frob things however we want to
for 3.16.

On that topic, qa has finally found the drv_suspend/forcewake issue. Chris
can you please pick out the minimal fix for that out of your tree? Maybe
on top of Paulo's fixes so that I don't have to wreak the patch applying
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch