Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
Op 08-07-15 om 10:12 schreef Patrik Jakobsson:
> On Tue, Jul 07, 2015 at 04:14:01PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 14:39 schreef Patrik Jakobsson:
>>> On Tue, Jul 07, 2015 at 12:22:12PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 11:18 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote:
>> This should fix suspend on newer platforms.
> Which patch broke this? Also what is "newer platform" and what exactly got
> fixed? Please elaborate a bit more in your commit messages, they're too
> terse.
There were a lot of warnings about active mismatches and power well not
being idle on suspend.
This should fix the power well by disabling the shared dpll and unsetting
crtc->active.
>>> This got broken by:
>>>
>>> commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6
>>> Author: Maarten Lankhorst
>>> Date: Mon Jun 15 12:33:53 2015 +0200
>>>
>>> drm/i915: Update less state during modeset.
>>>
>>> No need to repeatedly call update_watermarks, or update_fbc.
>>> Down to a single call to update_watermarks in .crtc_enable
>>>
>>> Signed-off-by: Maarten Lankhorst
>>> Reviewed-by: Matt Roper
>>> Tested-by(IVB): Matt Roper
>>> Signed-off-by: Daniel Vetter
>>>
>>> Unfortunately the patch doesn't fix the CAT_ERR on resume I'm experiencing
>>> on
>>> SKL. An additional intel_update_watermarks() is needed to set DDB back to
>>> 0,0.
>>> Also this is required in *_crtc_disable() since we forget to do the same
>>> thing
>>> there. Not sure we also need to take care of disabling fbc at these places?
>> I would prefer to have this fix, and leave updating the watermark code out
>> of crtc disable.
>>
>> Does it work If you add a intel_update_watermarks to the noatomic function?
> No that doesn't help. The only other callsite I can find is __intel_set_mode()
> so I guess watermarks need updating there as well.
Did you add it after the line that sets crtc->active = false?
And the watermark update is forced by intel_crtc_atomic_check, if you need 2 wm
updates post disable
that seems like a bug..
In any case below should work then.
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 6ddb462b4124..c1f7dfb62909 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6179,6 +6179,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
*crtc)
for_each_power_domain(domain, domains)
intel_display_power_put(dev_priv, domain);
intel_crtc->enabled_power_domains = 0;
+ intel_crtc->active = false;
+ intel_disable_shared_dpll(intel_crtc);
+ intel_update_watermarks(crtc);
}
/*
@@ -13158,7 +13161,8 @@ static int __intel_set_mode(struct drm_atomic_state
*state)
if (needs_modeset(crtc->state) && crtc->state->active) {
update_scanline_offset(to_intel_crtc(crtc));
dev_priv->display.crtc_enable(crtc);
- }
+ } else if (!crtc->state->active && crtc_state->active)
+ intel_update_watermarks(crtc);
drm_atomic_helper_commit_planes_on_crtc(crtc_state);
}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
On Tue, Jul 07, 2015 at 04:14:01PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 14:39 schreef Patrik Jakobsson: > > On Tue, Jul 07, 2015 at 12:22:12PM +0200, Maarten Lankhorst wrote: > >> Op 07-07-15 om 11:18 schreef Daniel Vetter: > >>> On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote: > This should fix suspend on newer platforms. > >>> Which patch broke this? Also what is "newer platform" and what exactly got > >>> fixed? Please elaborate a bit more in your commit messages, they're too > >>> terse. > >> There were a lot of warnings about active mismatches and power well not > >> being idle on suspend. > >> > >> This should fix the power well by disabling the shared dpll and unsetting > >> crtc->active. > > This got broken by: > > > > commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 > > Author: Maarten Lankhorst > > Date: Mon Jun 15 12:33:53 2015 +0200 > > > > drm/i915: Update less state during modeset. > > > > No need to repeatedly call update_watermarks, or update_fbc. > > Down to a single call to update_watermarks in .crtc_enable > > > > Signed-off-by: Maarten Lankhorst > > Reviewed-by: Matt Roper > > Tested-by(IVB): Matt Roper > > Signed-off-by: Daniel Vetter > > > > Unfortunately the patch doesn't fix the CAT_ERR on resume I'm experiencing > > on > > SKL. An additional intel_update_watermarks() is needed to set DDB back to > > 0,0. > > Also this is required in *_crtc_disable() since we forget to do the same > > thing > > there. Not sure we also need to take care of disabling fbc at these places? > I would prefer to have this fix, and leave updating the watermark code out of > crtc disable. > > Does it work If you add a intel_update_watermarks to the noatomic function? No that doesn't help. The only other callsite I can find is __intel_set_mode() so I guess watermarks need updating there as well. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
Op 07-07-15 om 14:39 schreef Patrik Jakobsson: > On Tue, Jul 07, 2015 at 12:22:12PM +0200, Maarten Lankhorst wrote: >> Op 07-07-15 om 11:18 schreef Daniel Vetter: >>> On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote: This should fix suspend on newer platforms. >>> Which patch broke this? Also what is "newer platform" and what exactly got >>> fixed? Please elaborate a bit more in your commit messages, they're too >>> terse. >> There were a lot of warnings about active mismatches and power well not >> being idle on suspend. >> >> This should fix the power well by disabling the shared dpll and unsetting >> crtc->active. > This got broken by: > > commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 > Author: Maarten Lankhorst > Date: Mon Jun 15 12:33:53 2015 +0200 > > drm/i915: Update less state during modeset. > > No need to repeatedly call update_watermarks, or update_fbc. > Down to a single call to update_watermarks in .crtc_enable > > Signed-off-by: Maarten Lankhorst > Reviewed-by: Matt Roper > Tested-by(IVB): Matt Roper > Signed-off-by: Daniel Vetter > > Unfortunately the patch doesn't fix the CAT_ERR on resume I'm experiencing on > SKL. An additional intel_update_watermarks() is needed to set DDB back to 0,0. > Also this is required in *_crtc_disable() since we forget to do the same thing > there. Not sure we also need to take care of disabling fbc at these places? I would prefer to have this fix, and leave updating the watermark code out of crtc disable. Does it work If you add a intel_update_watermarks to the noatomic function? ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
On Tue, Jul 07, 2015 at 12:22:12PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 11:18 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote: > >> This should fix suspend on newer platforms. > > Which patch broke this? Also what is "newer platform" and what exactly got > > fixed? Please elaborate a bit more in your commit messages, they're too > > terse. > There were a lot of warnings about active mismatches and power well not being > idle on suspend. > > This should fix the power well by disabling the shared dpll and unsetting > crtc->active. This got broken by: commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 Author: Maarten Lankhorst Date: Mon Jun 15 12:33:53 2015 +0200 drm/i915: Update less state during modeset. No need to repeatedly call update_watermarks, or update_fbc. Down to a single call to update_watermarks in .crtc_enable Signed-off-by: Maarten Lankhorst Reviewed-by: Matt Roper Tested-by(IVB): Matt Roper Signed-off-by: Daniel Vetter Unfortunately the patch doesn't fix the CAT_ERR on resume I'm experiencing on SKL. An additional intel_update_watermarks() is needed to set DDB back to 0,0. Also this is required in *_crtc_disable() since we forget to do the same thing there. Not sure we also need to take care of disabling fbc at these places? -Patrik ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
Op 07-07-15 om 11:18 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote: >> This should fix suspend on newer platforms. > Which patch broke this? Also what is "newer platform" and what exactly got > fixed? Please elaborate a bit more in your commit messages, they're too > terse. There were a lot of warnings about active mismatches and power well not being idle on suspend. This should fix the power well by disabling the shared dpll and unsetting crtc->active. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
On Tue, Jul 07, 2015 at 09:08:14AM +0200, Maarten Lankhorst wrote: > This should fix suspend on newer platforms. Which patch broke this? Also what is "newer platform" and what exactly got fixed? Please elaborate a bit more in your commit messages, they're too terse. -Daniel > > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/intel_display.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 6ddb462b4124..4d1a474e0d13 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6179,6 +6179,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc > *crtc) > for_each_power_domain(domain, domains) > intel_display_power_put(dev_priv, domain); > intel_crtc->enabled_power_domains = 0; > + intel_crtc->active = false; > + intel_disable_shared_dpll(intel_crtc); > } > > /* > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.
This should fix suspend on newer platforms. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/intel_display.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6ddb462b4124..4d1a474e0d13 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6179,6 +6179,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) for_each_power_domain(domain, domains) intel_display_power_put(dev_priv, domain); intel_crtc->enabled_power_domains = 0; + intel_crtc->active = false; + intel_disable_shared_dpll(intel_crtc); } /* -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
