From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs Date: Mon, 14 Jul 2014 19:34:41 +0200 Message-ID: <20140714173441.GJ15237@phenom.ffwll.local> References: <1403281762-1927-1-git-send-email-jbarnes@virtuousgeek.org> <20140707145313.719df3fd@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by gabe.freedesktop.org (Postfix) with ESMTP id DF0566E45C for ; Mon, 14 Jul 2014 10:34:30 -0700 (PDT) Received: by mail-wi0-f171.google.com with SMTP id hi2so3013018wib.10 for ; Mon, 14 Jul 2014 10:34:30 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch