From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Beckett Subject: Re: [PATCH 1/2] drm/i915: fix possible RPM ref leaking during RPS disabling Date: Thu, 22 May 2014 13:56:17 +0100 Message-ID: <537DF3F1.7080908@intel.com> References: <1399908905-3025-1-git-send-email-imre.deak@intel.com> <20140512175157.GC25056@phenom.ffwll.local> <1399988770.5451.3.camel@intelbox> <20140513135440.GI3908@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 7C2E36EBCA for ; Thu, 22 May 2014 05:56:20 -0700 (PDT) In-Reply-To: <20140513135440.GI3908@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 13/05/2014 14:54, Daniel Vetter wrote: > On Tue, May 13, 2014 at 04:46:10PM +0300, Imre Deak wrote: >> On Mon, 2014-05-12 at 19:51 +0200, Daniel Vetter wrote: >>> On Mon, May 12, 2014 at 06:35:04PM +0300, Imre Deak wrote: >>>> In >>>> >>>> commit c6df39b5ea6342323a42edfbeeca0a28c643d7ae >>>> Author: Imre Deak >>>> Date: Mon Apr 14 20:24:29 2014 +0300 >>>> >>>> drm/i915: get a runtime PM ref for the deferred GT powersave enabling >>>> >>>> I added an RPM get-ref when enabling RPS from a deferred work, but forgot >>>> to add the corresponding put-ref when canceling the work. This may leave >>>> RPM disabled. >>>> >>>> Signed-off-by: Imre Deak >>> >>> Could we intentionally hit this by e.g. racing a suspend/resume against >>> runtime pm? E.g. >>> 1. disable all outputs, make sure we've entered runtime pm >>> 2. set runtime autosuspend delay to 0 >>> 3. suspend/resume >>> 4. autosuspend (hopefully, my understanding is a bit unclear) >>> >>> ->Boom >>> >>> Would look nice as an igt subtest if it works ;-) >> >> Yep, works consistently as expected both before and after the fix. I >> pushed the new subtest, please add here: >> Testcase: igt/pm_pc8/system-suspend > > Now I'm confused: I expected this to blow up without your fix here, and > not work with or without it?! Please unconfuse ... > -Daniel > Im not sure what you are expecting to go boom. It is pretty difficult to get a race between suspend/resume and runtime pm. During system suspend, the pm core grabs a runtime ref without resume callbacks, and releases it on the resume. (see https://www.kernel.org/doc/Documentation/power/runtime_pm.txt part 6). Also, as i915_drm_freeze does a runtime pm get, at step 3, it will cause a runtime resume before doing the system suspend. On the system resume, it will do a runtime pm put, which will cause a runtime suspend due to autosuspend delay being 0, and so step 4, autosuspend will occur. This should all work fine (assuming the patch is applied to handle any refcounting between back to back resume->suspend which cancels the enable before it is executed). It all looks fine to me. Reviewed-by: Robert Beckett