From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended Date: Wed, 02 Apr 2014 01:35:22 +0300 Message-ID: <1396391722.2491.4.camel@ideak-mobl> References: <1396374913-2030-1-git-send-email-przanoni@gmail.com> <1396374913-2030-4-git-send-email-przanoni@gmail.com> <20140401203749.GB7225@phenom.ffwll.local> <20140401205259.GD7225@phenom.ffwll.local> <1396388208.2488.27.camel@ideak-mobl> <1396388570.2488.29.camel@ideak-mobl> <20140401220727.GH7225@phenom.ffwll.local> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 DD2C56E59A for ; Tue, 1 Apr 2014 15:35:25 -0700 (PDT) In-Reply-To: <20140401220727.GH7225@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: Daniel Vetter Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Wed, 2014-04-02 at 00:07 +0200, Daniel Vetter wrote: > On Wed, Apr 02, 2014 at 12:42:50AM +0300, Imre Deak wrote: > > On Wed, 2014-04-02 at 00:36 +0300, Imre Deak wrote: > > > On Tue, 2014-04-01 at 18:04 -0300, Paulo Zanoni wrote: > > > > 2014-04-01 17:52 GMT-03:00 Daniel Vetter : > > > > > On Tue, Apr 01, 2014 at 05:48:15PM -0300, Paulo Zanoni wrote: > > > > >> 2014-04-01 17:37 GMT-03:00 Daniel Vetter : > > > > >> > On Tue, Apr 01, 2014 at 02:55:09PM -0300, Paulo Zanoni wrote: > > > > >> >> From: Paulo Zanoni > > > > >> >> > > > > >> >> ... at edp_have_panel_vdd. Just return false, saying we don't have the > > > > >> >> panel VDD since the device is suspended. > > > > >> >> > > > > >> >> We started getting WARNs about this problem since the patch that > > > > >> >> started checking if we're suspended while reading registers. > > > > >> >> > > > > >> >> Testcase: igt/pm_pc8 > > > > >> >> Signed-off-by: Paulo Zanoni > > > > >> > > > > > >> > Hm, where's an example backtrace for this? I wonder whether we simply need > > > > >> > to extend the range where we hold the runtime pm ref a bit ... > > > > >> > > > > >> There are tons of WARNs, but here is one example: > > > > >> > > > > >> [ 63.572201] [drm:hsw_enable_pc8] Enabling package C8+ > > > > >> [ 63.581831] [drm:i915_runtime_suspend] Device suspended > > > > >> [ 63.664798] ------------[ cut here ]------------ > > > > >> [ 63.664824] WARNING: CPU: 3 PID: 828 at > > > > >> drivers/gpu/drm/i915/intel_uncore.c:47 > > > > >> assert_device_not_suspended.isra.7+0x32/0x40 [i915]() > > > > >> [ 63.664826] Device suspended > > > > >> [ 63.664828] Modules linked in: ccm fuse ip6table_filter ip6_tables > > > > >> ebtable_nat ebtables arc4 ath9k_htc ath9k_common ath9k_hw mac80211 ath > > > > >> cfg80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp > > > > >> microcode i2c_i801 e1000e pcspkr serio_raw lpc_ich ptp pps_core mei_me > > > > >> mei mfd_core dm_crypt i915 crc32_pclmul crc32c_intel > > > > >> ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video > > > > >> [ 63.664867] CPU: 3 PID: 828 Comm: kworker/3:3 Not tainted 3.14.0+ #153 > > > > >> [ 63.664869] Hardware name: Intel Corporation Shark Bay Client > > > > >> platform/WhiteTip Mountain 1, BIOS HSWLPTU1.86C.0133.R00.1309172123 > > > > >> 09/17/2013 > > > > >> [ 63.664887] Workqueue: events edp_panel_vdd_work [i915] > > > > >> [ 63.664889] 0000000000000009 ffff88009d745c28 ffffffff8167ec6f > > > > >> ffff88009d745c70 > > > > >> [ 63.664895] ffff88009d745c60 ffffffff8106c8ed ffff880036278000 > > > > >> 00000000000c7204 > > > > >> [ 63.664900] ffff88014f2d3040 ffff880036278070 0000000000000001 > > > > >> ffff88009d745cc0 > > > > >> [ 63.664905] Call Trace: > > > > >> [ 63.664911] [] dump_stack+0x4d/0x66 > > > > >> [ 63.664916] [] warn_slowpath_common+0x7d/0xa0 > > > > >> [ 63.664920] [] warn_slowpath_fmt+0x4c/0x50 > > > > >> [ 63.664926] [] ? mark_held_locks+0xae/0x130 > > > > >> [ 63.664941] [] > > > > >> assert_device_not_suspended.isra.7+0x32/0x40 [i915] > > > > >> [ 63.664956] [] gen6_read32+0x32/0x120 [i915] > > > > >> [ 63.664969] [] ? gen6_read8+0x120/0x120 [i915] > > > > >> [ 63.664985] [] edp_have_panel_vdd+0x3f/0x50 [i915] > > > > >> [ 63.665000] [] edp_panel_vdd_off_sync+0x58/0x1c0 [i915] > > > > >> [ 63.665004] [] ? process_one_work+0x18c/0x560 > > > > >> [ 63.665018] [] edp_panel_vdd_work+0x34/0x50 [i915] > > > > >> [ 63.665022] [] process_one_work+0x1f7/0x560 > > > > >> [ 63.665026] [] ? process_one_work+0x18c/0x560 > > > > >> [ 63.665031] [] worker_thread+0x11b/0x3a0 > > > > > > > > > > Ah, that's the async edp worker thread. I guess we need to grab a runtim > > > > > pm ref when we start it and drop it again in the worker instead? > > > > > > > > IMHO it doesn't make sense to keep the HW awake just so we can read a > > > > register to find out things are disabled, so I prefer the current > > > > solution. If we want a "better" solution, IMHO we should track the > > > > sate of the VDD in software, like intel_dp->has_panel_vdd. This would > > > > avoid many more register reads. > > Yeah, we can also try to cancel the work if we go into runtime pm. But > otoh panel vdd is kinda like a power domain of it's own, so we should > apply the usual nesting rules that power domains grab references for the > outer power domains they depend on. Panel vdd without runtime pm probably > isn't a possible comibination, or at least not one that makes sense ;-) Well this nesting is already provided, since we get the port power domain in edp_panel_vdd_on() and through that an RPM ref. > -Daniel > > > > > > > Chiming in, as I was wondering what could cause the inbalance between > > > the edp_panel_vdd_on() and off() calls. One possibility is that > > > intel_dp_probe_oui() calls edp_panel_vdd_on() and then > > > edp_panel_vdd_off(..., false) which schedules the work and then > > > intel_enable_dp() calls edp_panel_vdd_on() and edp_panel_vdd_off(..., > > > true) and disables VDD synchronously, dropping the RPM ref. Note that in > > > this case we won't get the "eDP VDD not forced on" WARN either. > > > > > > Whether or not this was the case for you, I think for good measure we > > > should flush any pending vdd_work in _edp_panel_vdd_on() before setting > > > intel_dp->want_panel_vdd = true; > > > > Actually not flush but cancel the work. > > > > --Imre > > > > >