* [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
@ 2013-07-11 8:02 Xiong Zhang
2013-07-11 16:53 ` Jesse Barnes
0 siblings, 1 reply; 5+ messages in thread
From: Xiong Zhang @ 2013-07-11 8:02 UTC (permalink / raw)
To: intel-gfx
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@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);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
2013-07-11 8:02 [PATCH] drm/i915: Decrease pll->refcount when freeze gpu Xiong Zhang
@ 2013-07-11 16:53 ` Jesse Barnes
2013-07-11 17:31 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Jesse Barnes @ 2013-07-11 16:53 UTC (permalink / raw)
To: Xiong Zhang; +Cc: intel-gfx
On Thu, 11 Jul 2013 16:02:27 +0800
Xiong Zhang <xiong.y.zhang@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@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...
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
2013-07-11 16:53 ` Jesse Barnes
@ 2013-07-11 17:31 ` Daniel Vetter
2013-07-12 8:11 ` Zhang, Xiong Y
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2013-07-11 17:31 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Thu, Jul 11, 2013 at 6:53 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 11 Jul 2013 16:02:27 +0800
> Xiong Zhang <xiong.y.zhang@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
2013-07-11 17:31 ` Daniel Vetter
@ 2013-07-12 8:11 ` Zhang, Xiong Y
2013-07-12 19:25 ` Jesse Barnes
0 siblings, 1 reply; 5+ messages in thread
From: Zhang, Xiong Y @ 2013-07-12 8:11 UTC (permalink / raw)
To: Daniel Vetter, Jesse Barnes; +Cc: intel-gfx
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@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@ffwll.ch [mailto:daniel.vetter@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@virtuousgeek.org> wrote:
> On Thu, 11 Jul 2013 16:02:27 +0800
> Xiong Zhang <xiong.y.zhang@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
2013-07-12 8:11 ` Zhang, Xiong Y
@ 2013-07-12 19:25 ` Jesse Barnes
0 siblings, 0 replies; 5+ messages in thread
From: Jesse Barnes @ 2013-07-12 19:25 UTC (permalink / raw)
To: Zhang, Xiong Y; +Cc: intel-gfx
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@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@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@ffwll.ch [mailto:daniel.vetter@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@virtuousgeek.org> wrote:
> > On Thu, 11 Jul 2013 16:02:27 +0800
> > Xiong Zhang <xiong.y.zhang@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-12 19:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-11 8:02 [PATCH] drm/i915: Decrease pll->refcount when freeze gpu Xiong Zhang
2013-07-11 16:53 ` Jesse Barnes
2013-07-11 17:31 ` Daniel Vetter
2013-07-12 8:11 ` Zhang, Xiong Y
2013-07-12 19:25 ` Jesse Barnes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.