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 maarten.lankho...@linux.intel.com 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 maarten.lankho...@linux.intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Tested-by(IVB): Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch 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 maarten.lankho...@linux.intel.com 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 maarten.lankho...@linux.intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Tested-by(IVB): Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch 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.
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 maarten.lankho...@linux.intel.com --- 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
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 maarten.lankho...@linux.intel.com 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 maarten.lankho...@linux.intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Tested-by(IVB): Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch 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 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 maarten.lankho...@linux.intel.com 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 maarten.lankho...@linux.intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Tested-by(IVB): Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch 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