2014-07-17 5:06 GMT-03:00 Daniel Vetter <daniel@xxxxxxxx>: > On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote: >> 2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel@xxxxxxxx>: >> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote: >> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> >> > On Mon, 7 Jul 2014 18:48:47 -0300 >> >> > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: >> >> > >> >> >> (documenting what we discussed on IRC) >> >> >> >> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> >> >> > This was always the case on our suspend path, but it was recently >> >> >> > exposed by the change to use our runtime IRQ disable routine rather than >> >> >> > the full DRM IRQ disable. Keep the warning on the enable side, as that >> >> >> > really would indicate a bug. >> >> >> >> >> >> While I understand the patch and think it's a reasonable thing to do, >> >> >> I feel the need to spend some time persuading you in replacing it with >> >> >> something that doesn't involve removing WARNs from our driver. While >> >> >> the driver is runtime suspended, no one should really be manipulating >> >> >> IRQs, even if they're disabling stuff that is already disabled: this >> >> >> reflects there's probably a problem somewhere. These WARNs are >> >> >> extremely helpful for the runtime PM feature because almost nobody >> >> >> actually uses runtime PM to notice any bugs with it, so the WARNs can >> >> >> make QA report bugs and bisect things for us. >> >> >> >> >> >> Also, it seems S3 suspend is currently a little disaster on our >> >> >> driver. Your 6 patches just solve some of the WARNs, not all of them. >> >> >> And last week I even solved another WARN on the S3 path. I just did >> >> >> some investigation, and the current bad commits are: >> >> >> 8abdc17941c71b37311bb93876ac83dce58160c8, >> >> >> e11aa362308f5de467ce355a2a2471321b15a35c and >> >> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3 >> >> >> commits, S3 doesn't give me any WARNs. >> >> >> >> >> >> Instead of the change you proposed, can't we think of another solution >> >> >> that would maybe address all the 3 regressions we have? Since we're >> >> >> always submitting patches to change the order we do things at S3 >> >> >> suspend/resume, shouldn't we add something like "dev_priv->suspending" >> >> >> that could be used to avoid the strict ordering that is required >> >> >> during runtime? >> >> > >> >> > Hm I was running with those changes and didn't see additional warnings, >> >> > so I'll have to look again. >> >> > >> >> > I agree we want sensible warnings in place, and maybe removing this one >> >> > is too permissive, but I'd like to avoid a suspending flag if we can. >> >> > >> >> > Maybe we just need to bundle up our assertions and check all the >> >> > relevant state after runtime suspend or S3 like we do for mode set >> >> > state in various places. That would let us keep our warnings, but also >> >> > save us from having them spread out in code paths that get used in >> >> > many different contexts. >> >> >> >> I'm probably going to regret this, but since no one proposed a better >> >> patch and the bug is still there: >> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> > >> > Not really eager to merge a patch soaking in preemptive regrets ... >> > >> > I'll punt on this for now. >> >> Possible solutions: >> >> 1 - Patch xxx_disable_vblank to do nothing in case dev_priv->pm.suspended >> 2 - Add a flag dev_priv->suspending and don't print those WARNs in >> case it is set. >> 3 - Merge Jesse's original patch >> 4 - Something else? >> >> I can implement any of the proposed solutions if needed... > > I've gone with 3 for now. It sounds like we need to work with this code a > bit more still until the best solution is clear. The patch at least > addresses the WARN. Ok, so after I wrote this email, I remembered Ville's "[10/24] drm/i915: Leave interrupts enabled while disabling crtcs during suspend", which is part of the watermarks series. Based on that, I produced the following fix that would replace Jesse's current patch series, fixing the bad WARN while keeping it in the places we want: diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3315358..63675bf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev) return error; } - flush_delayed_work(&dev_priv->rps.delayed_resume_work); - - intel_runtime_pm_disable_interrupts(dev); - - intel_suspend_gt_powersave(dev); - /* * Disable CRTCs directly since we want to preserve sw state * for _thaw. Also, power gate the CRTC power wells. @@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev) intel_crtc_control(crtc, false); drm_modeset_unlock_all(dev); + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + intel_runtime_pm_disable_interrupts(dev); + + intel_suspend_gt_powersave(dev); + intel_modeset_suspend_hw(dev); } What do you think? > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx