Re: [PATCH 0/9] Haswell Package C8+

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 06, 2013 at 06:57:10PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> Here's my next attempt at the Haswell PC8+ feature.
> 
> What's new?
> 
>  1. I hope I implemented the feedback from Chris. Many function renames, a few
>     style changes. Chris' biggest concern was about the interrupts. So in this
>     series you'll see patches 2-6 moving some interrupt code around, so all
>     changes to each IMR register should go through a single function. With this
>     change we can easily detect when someone wants to change IMR but interrupts
>     are disabled. As he suggested, we also don't enable the interrupts in that
>     case.
>  2. Another difference is that now we also disable PC8 when there's GPU work to
>     do. The lack of interrupts made things like glxgears work at 2 FPS. I talked
>     to Ben about this and what I understood is that the lack of interrupts is
>     not really a problem, it just makes things slower. But still I decided to
>     write the code to disable PC8 when we have GPU work to do, so background
>     apps can run faster than 2 FPS.

Ben just mislead you here, this is only true due to an implemenation
detail on our hw simulation enviroment. On real hw running gem workloads
without interrupt support very much blows up as soon as something hits
__wait_seqno and actually goes to sleep on the waitqueue.

If you pimp the your igt testcase in the test_batch code with the dummy
workload and wait_rendering stuff you should be able to hit this fairly
easily. I haven't looked yet but I think adding a "are interrupts
working/am I out of pc8+" check to __wait_seqno would be good.
-Daniel

>  3. Another difference is that now we enable PC8 on a delayed work function that
>     only gets called if we stay with 0 refcount for at least 5 seconds. So when
>     someone runs "xrandr" we won't enable/disable PC8 dozens of times. Also, on
>     "xset dpms force off" we'll only get to PC8 if the desktop environment
>     decides to not do rendering every second. Not getting to PC8? Fix your
>     desktop environment!
>  4. Due to 3 and the fact that Daniel didn't seem to like my "do DP AUX and
>     GMBUS without interrupts" approach, the previous patches that implemented
>     this just got dropped: we now have the delayed work thing which should
>     replace them.
>  5. I also added code to wake up from PC8 when doing suspend, we need this.
> 
> Despite this, the other thing to discuss is about the size of patch 7. I can
> certainly try to split it, but I really think that if all you want is to take a
> brief look at the code just and drop some bikesheds, then having a big patch
> that implements everything in one piece seems better. I can split this later if
> needed. I'm also open to ideas on how to really split this patch. Also notice
> that even if the patch is big, it doesn't remove a single line of the current
> code. And it adds PC8 disabled by default, so if git bisect points to that patch
> the surface to look will be small.
> 
> Another thing worth mentioning is that all this code doesn't have IS_HASWELL
> checks, and on non-Haswell platforms the refcount will never reach 0, so we
> won't ever try to enable PC8. I'm not sure if that's what we want, so please
> comment on that.
> 
> Even if we decide to delay patch 7, we could try to merge patches 1-6 if they
> look acceptable. I'm also not really sure if we want the last patch yet, but
> it's there just in case.
> 
> Also, I couldn't do i-g-t regression testing since the i-g-t test suite is quite
> unreliable on Haswell right now.
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (9):
>   drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
>   drm/i915: wrap GTIMR changes
>   drm/i915: wrap GEN6_PMIMR changes
>   drm/i915: don't update GEN6_PMIMR when it's not needed
>   drm/i915: add dev_priv->pm_irq_mask
>   drm/i915: don't disable/reenable IVB error interrupts when not needed
>   drm/i915: allow package C8+ states on Haswell (disabled)
>   drm/i915: add i915_pc8_status debugfs file
>   drm/i915: enable Package C8+ by default
> 
>  drivers/gpu/drm/i915/i915_debugfs.c     |  25 ++++
>  drivers/gpu/drm/i915/i915_dma.c         |  10 ++
>  drivers/gpu/drm/i915/i915_drv.c         |  11 ++
>  drivers/gpu/drm/i915/i915_drv.h         |  73 ++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c         | 202 +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_ddi.c        |   9 +-
>  drivers/gpu/drm/i915/intel_display.c    | 164 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c         |   3 +
>  drivers/gpu/drm/i915/intel_drv.h        |  14 +++
>  drivers/gpu/drm/i915/intel_i2c.c        |   2 +
>  drivers/gpu/drm/i915/intel_pm.c         |  13 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  32 ++---
>  12 files changed, 518 insertions(+), 40 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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