Re: [Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
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 +
"Zhang, Xiong Y" 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
>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.vet...@ffwll.ch [mailto:daniel.vet...@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
> wrote:
> > On Thu, 11 Jul 2013 16:02:27 +0800
> > Xiong Zhang 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
> >> ---
> >> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
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
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.vet...@ffwll.ch [mailto:daniel.vet...@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 wrote:
> On Thu, 11 Jul 2013 16:02:27 +0800
> Xiong Zhang 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
>> ---
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
On Thu, Jul 11, 2013 at 6:53 PM, Jesse Barnes wrote:
> On Thu, 11 Jul 2013 16:02:27 +0800
> Xiong Zhang 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
>> ---
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
On Thu, 11 Jul 2013 16:02:27 +0800
Xiong Zhang 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
> ---
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Decrease pll->refcount when freeze gpu
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
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
