[Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu

[Jesse Barnes]

So this is a dupe of
https://bugs.freedesktop.org/show_bug.cgi?id=64379, which is a bit
trickier than just the switchless stuff, as it indicates we're not
tracking PLL state correctly...

I thought Daniel had patches for that; I know I had some for IVB, but
they didn't apply to HSW, and those have been made obsolete by patches
from Daniel that are upstream.  Maybe we're just missing hsw bits?

Jesse

On Fri, 12 Jul 2013 08:11:14 +0000
"Zhang, Xiong Y" <xiong.y.zhang at intel.com> wrote:

> Hi Jesse:
>   I supply this patch because I encounter the S3 and S4 problem on Haswell connecting VGA or HDMI screen.
>   Currently nobody call intel_ddi_put_crtc_pll() to decrease pll_refcount and clear ddi_pll_sel when enter sleep states.
>   So when resume from sleep states, pll_refcount is larger than zero. mode setting function will call intel_ddi_pll_mode_set().
>   Intel_ddi_pll_mode_set call intel_ddi_put_crtc_pll() first, then set pll and increase pll-refcount. The results are:
>   1. S3 resume have call trace in intel_ddi_put_crtc_pll()
>     If connecting vga, the call trace is "WARN_ON(!SPLL_PLL_ENABLE)"
>     If connecting HDMI, the call trace is "WARN_ON(!WRPLL_PLL_ENABLE)"
>   2. S4 resume fail in intel_ddi_pll_mode_set()
>     If connecting VGA, intel_ddi_pll_mode_set () return false and mode setting exit without setting mode, vga is black
>     If connecting HDMI, before enter S4, HDMI use WRPLL1.After resume from S4, HDMI use WRPLL2.  The status is different during S4
> 
>  Actually, the above problem is a regression caused by your commit:
>    commit 24576d23976746cb52e7700c4cadbf4bc1bc3472
>    Author: Jesse Barnes <jbarnes at virtuousgeek.org>
>    Date:   Tue Mar 26 09:25:45 2013 -0700
>      drm/i915: enable VT switchless resume v3
>   
> In your patch, you delete intel_modeset_disable() from i915_drm_freeze(), intel_modeset_disable() will call dev_priv->display.off(crtc) to 
> decrease pll_refcount and disable PLL.
> 
> My question is whether PLL can be disabled when enable VT switchless ?
> 
> Thanks
> 
> -----Original Message-----
> From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Friday, July 12, 2013 1:32 AM
> To: Jesse Barnes
> Cc: Zhang, Xiong Y; intel-gfx
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
> 
> On Thu, Jul 11, 2013 at 6:53 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> > On Thu, 11 Jul 2013 16:02:27 +0800
> > Xiong Zhang <xiong.y.zhang at intel.com> wrote:
> >
> >> display.crtc_mode_set will increase pll->refcount, but no one will 
> >> decrease pll->refcount when freeze gpu. So when gpu resume from 
> >> freeze,
> >> pll->refcount is still larger than zero. This is abnormal
> >>
> >> Without this patch, connecting vga screen on Haswell platform, there 
> >> are following results:
> >> 1. when resume S3, call trace exist in intel_ddi_put_crtc_pll() 2. 
> >> when resume s4, vga monitor is black. because intel_ddi_pll_mode_set()
> >>    return false and haswell_crtc_mode_set() exit without setting mode
> >>
> >> With this patch, I don't find S3 and S4 regression on SandyBridge and 
> >> IvyBridge platform connecting VGA, HDMI and DP screen.
> >>
> >> Signed-off-by: Xiong Zhang <xiong.y.zhang at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.c |    4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> >> b/drivers/gpu/drm/i915/i915_drv.c index 0485f43..0065735 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -575,8 +575,10 @@ static int i915_drm_freeze(struct drm_device *dev)
> >>                * Disable CRTCs directly since we want to preserve sw state
> >>                * for _thaw.
> >>                */
> >> -             list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> >> +             list_for_each_entry(crtc, &dev->mode_config.crtc_list, 
> >> + head) {
> >>                       dev_priv->display.crtc_disable(crtc);
> >> +                     dev_priv->display.off(crtc);
> >> +             }
> >>
> >>               intel_modeset_suspend_hw(dev);
> >>       }
> >
> > The comment above this call indicates we'll trash the sw state if we 
> > call ->off directly.  Does suspend/resume still work both with and 
> > without X with this patch applied?  If we trash the sw state, the VT 
> > switchless resume shouldn't work...
> 
> Even without that little issue: ddi refcounting issue need to be fixed in the haswell platform code, not by papering over in the core modeset infrastructure. We have refcounted pch plls on snb/ivb, and it works.
> So imo there's no issue with the core code in the driver.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 


-- 
Jesse Barnes, Intel Open Source Technology Center