| Message ID | 1373529747-29943-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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
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
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 >
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); }
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(-)