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

[Zhang, Xiong Y]

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