From patchwork Thu Jul 17 13:42:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paulo Zanoni X-Patchwork-Id: 4575481 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0D583C0514 for ; Thu, 17 Jul 2014 13:43:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0A42E200F2 for ; Thu, 17 Jul 2014 13:43:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id DA40F2018E for ; Thu, 17 Jul 2014 13:42:58 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 384B86E22A; Thu, 17 Jul 2014 06:42:58 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ig0-f182.google.com (mail-ig0-f182.google.com [209.85.213.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 0CAFF6E22A for ; Thu, 17 Jul 2014 06:42:55 -0700 (PDT) Received: by mail-ig0-f182.google.com with SMTP id c1so2847323igq.3 for ; Thu, 17 Jul 2014 06:42:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=aiySyPzr/V+DBfQ74QIqpSph+1ZubWik2k7I1Lf60WM=; b=vRqz6fotrVpj2Iwqm+umUoSvYSRgtX00EGIrH3vLjtRghnY6YVh2KZY+7ZiYy+BWk2 4z/RGQyLomsL+BYE5nKt703H7tOuersu5qq16ehH2PV/RQlmB7h3sZFE7OkEsfZCz/w4 7DQetnfyJQAR1aCikB0GX6FTZvy/o5XpnNRW28qTusbFDreQ7xznFg03qYynsMWwNNCa z6AbgUc2UxUdIztbzigq9sVOVvA3wmscsrGAkJRNnmgs48Qjcj+T7duH9Izrv6/OiqQU X6tqqfOop6x6LWys/QHdIM1y1gze+J3DSfdDtb+oZL1Z3/0DUrxTcx/BcgLyrSrHjrio eyrg== MIME-Version: 1.0 X-Received: by 10.60.134.76 with SMTP id pi12mr34780362oeb.0.1405604575307; Thu, 17 Jul 2014 06:42:55 -0700 (PDT) Received: by 10.76.34.162 with HTTP; Thu, 17 Jul 2014 06:42:55 -0700 (PDT) In-Reply-To: <20140717080627.GA15237@phenom.ffwll.local> References: <1403281762-1927-1-git-send-email-jbarnes@virtuousgeek.org> <20140707145313.719df3fd@jbarnes-desktop> <20140714173441.GJ15237@phenom.ffwll.local> <20140717080627.GA15237@phenom.ffwll.local> Date: Thu, 17 Jul 2014 10:42:55 -0300 Message-ID: From: Paulo Zanoni To: Daniel Vetter Cc: Intel Graphics Development Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2014-07-17 5:06 GMT-03:00 Daniel Vetter : > On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote: >> 2014-07-14 14:34 GMT-03:00 Daniel Vetter : >> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote: >> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes : >> >> > On Mon, 7 Jul 2014 18:48:47 -0300 >> >> > Paulo Zanoni wrote: >> >> > >> >> >> (documenting what we discussed on IRC) >> >> >> >> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes : >> >> >> > 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 >> > >> > 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: What do you think? > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch 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); }