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

[Daniel Vetter]

On Thu, Jul 17, 2014 at 3:42 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>> 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?

Yeah, that means we can keep using the power domain code when shutting
down pipes, so more common code. And we need it anyway to make Ville
happy. Since I'm only going to freeze down dinq next week for -testing
I'll keep Jesse patches for now until he's back from vacation and can
review your patch. But please polish this one with commit message and
all.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch