From patchwork Sun Feb 17 13:38:14 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 2153191 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id E0B35DF283 for ; Sun, 17 Feb 2013 13:36:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9823AE6035 for ; Sun, 17 Feb 2013 05:36:10 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 71B43E5CB7 for ; Sun, 17 Feb 2013 05:35:57 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id n13so1934252eaa.8 for ; Sun, 17 Feb 2013 05:35:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=x-received:date:from:to:cc:subject:message-id:mail-followup-to :references:mime-version:content-type:content-disposition :in-reply-to:x-originating-ip:user-agent; bh=W6M2Wd7LW7w7NBn0rFSQwgx3lYkDmozBeTn3UI9x4yc=; b=Qlz4uLjCfVW7Yad/1NTtHyMvZICePpdoqM4uiEQGS3bsWS2RzHQygDG7DNEhrervqu 5KObMOk2rxmpyyVHimI/N3M+JxG+q5U9Kx5FwJo/OHUP9nj/NgXgZbT6M+NVeurQJgyx LZTD4LEVLkkKZi7dx0JZCuIojfLljzNXqL92g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:mail-followup-to :references:mime-version:content-type:content-disposition :in-reply-to:x-originating-ip:user-agent:x-gm-message-state; bh=W6M2Wd7LW7w7NBn0rFSQwgx3lYkDmozBeTn3UI9x4yc=; b=eCtpn9GzeG40n3+AxjO2ZaC1jtBKIdgkUHZ/X52l8KLz2iMYmdyQdsaUXWHtlYkc+g JnpKIJI6i7cs+eg4ppKJDb60vBaMm9Mij4vdxLz+cAB3oOWPyTfMzt4h2dBJfa8Z4hQY 6MImzon5cwG+W7Qegc3qZzxhLJRloGa/4GmXi78c3qiioChFiUnxc8olzPsBoe/oP1E5 GjrBRDfpmoHUu7RSQvz7aw3JZuoXEU+2g/S6MJJBj9cH3bjKB8OGu7VVIlwScrnuTDl5 NIO2tK4xtpCf1gdr478UpPNc/bSFNY03uOpQkF2dYWcj6rgR8dTa6bY52TVUhOWEDL8z XKSA== X-Received: by 10.14.204.195 with SMTP id h43mr32838386eeo.14.1361108156226; Sun, 17 Feb 2013 05:35:56 -0800 (PST) Received: from phenom.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id q42sm84112056eem.14.2013.02.17.05.35.54 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 17 Feb 2013 05:35:55 -0800 (PST) Date: Sun, 17 Feb 2013 14:38:14 +0100 From: Daniel Vetter To: Hugh Dickins Subject: Re: Debugging Thinkpad T430s occasional suspend failure. Message-ID: <20130217133814.GK5813@phenom.ffwll.local> Mail-Followup-To: Hugh Dickins , Linus Torvalds , David Airlie , Dave Jones , Linux Kernel Mailing List , Paul McKenney , DRI References: <20130213193411.GA15928@redhat.com> <20130215011503.GA11914@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Received: by 10.43.5.74 with HTTP; Sun, 17 Feb 2013 05:27:33 -0800 (PST) X-Originating-IP: [178.83.130.250] User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQn9GJ66hmj6uVkwAWYDWMAy3ORO1P0BtwpQcy2IWFK+6IShyDdXEUYS9S/sTLjqvdvdWNVX Cc: Linux Kernel Mailing List , DRI , Paul McKenney , Dave Jones , Linus Torvalds X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org On Sun, Feb 17, 2013 at 3:21 AM, Hugh Dickins wrote: > On Sat, 16 Feb 2013, Hugh Dickins wrote: >> On Sat, 16 Feb 2013, Linus Torvalds wrote: >> > >> > I think it's worth it to give them a heads-up already. So I've cc'd >> > the main suspects here.. >> >> Okay, thanks. >> >> > >> > Daniel, Dave - any comments about a NULL fb in >> > intel_choose_pipe_bpp_dither() during either suspend or resume? Some >> > googling shows this: >> > >> > https://bugzilla.redhat.com/show_bug.cgi?id=895123 >> >> Great, yes, I'm sure that's the same (though it says "suspend" >> and I say "resume"). >> >> > >> > which sounds remarkably similar, and is also during a suspend attempt >> > (but apparently Satish got a full oops out).. Some timing race with a >> > worker entry? > > Comparing Satish's backtrace and drivers/gpu/drm history, it's clear that > the oops comes from Daniel's 3.8-rc2 45e2b5f640b3 "drm/i915: force restore > on lid open", whose force_restore case now passes down crtc->base.fb. But > I wouldn't have a clue why that's usually non-NULL but occasionally NULL: > your timing race with a worker entry, perhaps. > > And 45e2b5f640b3 contains a fine history of going back and forth, so I > wouldn't want to play further with it out of ignorance - though tempted > to replace the "if (force_restore) {" by an interim safe-seeming > compromise of "if (force_restore && crtc->base.fb) {". Two things to try while I try to grow a clue on what exactly is going on: 1. Related to new ACPI sleep states we've recently made the lid_notifier locking more sound, maybe that helps: http://cgit.freedesktop.org/~danvet/drm-intel/commit/?h=drm-intel-next-queued&id=b8efb17b3d687695b81485f606fc4e6c35a50f9a 2. The new i915 force restore code is indeed missing a safety check compared to the old crtc helper based one: The issue is that we save a pointer to the fb (since those are refcounted) but copy the mode into the crtc struct (since modes are not refcounted). So on restore the mode will always be non-NULL, which is wrong if the crtc is off (and so also has a NULL fb). The problem I have with that patch is figuring out how this ever worked. I think I need more coffee ;-) Cheers, Daniel diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6eb3882..095094c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9153,7 +9153,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, if (force_restore) { for_each_pipe(pipe) { - intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]); + struct drm_crtc * crtc = + dev_priv->pipe_to_crtc_mapping[pipe]; + + if (!crtc->enabled) + continue; + + intel_crtc_restore_mode(crtc); } i915_redisable_vga(dev);