[Intel-gfx] [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Paulo Zanoni]

2014-07-17 5:06 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote:
>> 2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
>> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
>> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> >> > On Mon, 7 Jul 2014 18:48:47 -0300
>> >> > Paulo Zanoni <przanoni at gmail.com> wrote:
>> >> >
>> >> >> (documenting what we discussed on IRC)
>> >> >>
>> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> >> >> > 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 at intel.com>
>> >
>> > 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