Re: [PATCH v2 03/20] drm/i915: Fix noatomic crtc disabling.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.lankhorst@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.lankhorst@linux.intel.com>
>>>     Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>     Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com>
>>>     Signed-off-by: Daniel Vetter <daniel.vetter@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