Re: [PATCH] drm/i915: Stop calling intel_prepare_ddi during power well initialization.

From: Imre Deak <imre.deak@intel.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "Shah, Suketu J" <suketu.j.shah@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: Stop calling intel_prepare_ddi during power well initialization.
Date: Wed, 23 Sep 2015 20:48:32 +0300	[thread overview]
Message-ID: <1443030512.28496.93.camel@intel.com> (raw)
In-Reply-To: <AC49CA744796934484854BF79A8D0E56487B8F5F@FMSMSX113.amr.corp.intel.com>

On ke, 2015-09-23 at 17:34 +0000, Vivi, Rodrigo wrote:
> here goes one trace...
> To be honest I'm confused as well: looking to the code that initializing boolean should be protecting this case...
> And if I remove this first one it will end up on that other at post enable... 

It is the one in skl_power_well_post_enable(), since that doesn't check
for the flag.

> 
> But apparently this only happens when something didn't go well with power well initialization...
> 
> so, besides the initialization and resume from s3 this path here happens on every wake up from DC5/DC6?
> 
> [    9.618747] i915 0000:00:02.0: Invalid ROM contents
> [    9.631446] [drm] failed to find VBIOS tables
> [    9.720036] BUG: unable to handle kernel NULL pointer dereference at 00000000
> 00000058
> [    9.721986] IP: [<ffffffffa014eb72>] ddi_get_encoder_port+0x82/0x190 [i915]
> [    9.723736] PGD 0 
> [    9.724286] Oops: 0000 [#1] PREEMPT SMP 
> [    9.725386] Modules linked in: intel_powerclamp snd_hda_intel(+) coretemp crc
> 32c_intel snd_hda_codec snd_hda_core serio_raw snd_pcm snd_timer i915(+) parport
> _pc parport pinctrl_sunrisepoint pinctrl_intel nfsd nfs_acl
> [    9.730635] CPU: 0 PID: 497 Comm: systemd-udevd Not tainted 4.3.0-rc2-eywa-10
> 967-g72de2cfd-dirty #2
> [    9.732785] Hardware name: Intel Corporation Cannonlake Client platform/Skyla
> ke DT DDR4 RVP8, BIOS CNLSE2R1.R00.X021.B00.1508040310 08/04/2015
> [    9.735785] task: ffff88008a704700 ti: ffff88016a1ac000 task.ti: ffff88016a1a
> c000
> [    9.737584] RIP: 0010:[<ffffffffa014eb72>]  [<ffffffffa014eb72>] ddi_get_enco
> der_port+0x82/0x190 [i915]
> [    9.739934] RSP: 0000:ffff88016a1af710  EFLAGS: 00010296
> [    9.741184] RAX: 000000000000004e RBX: ffff88008a9edc98 RCX: 0000000000000001
> [    9.742934] RDX: 000000000000004e RSI: ffffffff81fc1e82 RDI: 00000000ffffffff
> [    9.744634] RBP: ffff88016a1af730 R08: 0000000000000000 R09: 0000000000000578
> [    9.746333] R10: 0000000000001065 R11: 0000000000000578 R12: fffffffffffffff8
> [    9.748033] R13: ffff88016a1af7a8 R14: ffff88016a1af794 R15: 0000000000000000
> [    9.749733] FS:  00007eff2e1e07c0(0000) GS:ffff88016fc00000(0000) knlGS:00000
> 00000000000
> [    9.751683] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    9.753083] CR2: 0000000000000058 CR3: 000000016922b000 CR4: 00000000003406f0
> [    9.754782] Stack:
> [    9.755332]  ffff88008a9edc98 ffff88008a9ed800 ffffffffa01d07b0 00000000fffb9
> 09e
> [    9.757232]  ffff88016a1af7d8 ffffffffa0154ea7 0000000000000246 ffff88016a370
> 080
> [    9.759182]  ffff88016a370080 ffff88008a9ed800 0000000000000246 ffff88008a9ed
> c98
> [    9.761132] Call Trace:
> [    9.761782]  [<ffffffffa0154ea7>] intel_prepare_ddi+0x67/0x860 [i915]
> [    9.763332]  [<ffffffff81a56996>] ? _raw_spin_unlock_irqrestore+0x26/0x40
> [    9.765031]  [<ffffffffa00fad01>] ? gen9_read32+0x141/0x360 [i915]
> [    9.766531]  [<ffffffffa00b43e1>] skl_set_power_well+0x431/0xa80 [i915]
> [    9.768181]  [<ffffffffa00b4a63>] skl_power_well_enable+0x13/0x20 [i915]
> [    9.769781]  [<ffffffffa00b2188>] intel_power_well_enable+0x28/0x50 [i915]
> [    9.771481]  [<ffffffffa00b4d52>] intel_display_power_get+0x92/0xc0 [i915]
> [    9.773180]  [<ffffffffa00b4fcb>] intel_display_set_init_power+0x3b/0x40 [i91
> 5]
> [    9.774980]  [<ffffffffa00b5170>] intel_power_domains_init_hw+0x120/0x520 [i9
> 15]
> [    9.776780]  [<ffffffffa0194c61>] i915_driver_load+0xb21/0xf40 [i915]
> 
> 
> ________________________________________
> From: Daniel Vetter [daniel.vetter@ffwll.ch] on behalf of Daniel Vetter [daniel@ffwll.ch]
> Sent: Wednesday, September 23, 2015 8:58 AM
> To: Vivi, Rodrigo
> Cc: intel-gfx@lists.freedesktop.org; Shah, Suketu J
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Stop calling intel_prepare_ddi during power well initialization.
> 
> On Tue, Sep 22, 2015 at 08:03:59AM -0700, Rodrigo Vivi wrote:
> > The comment removed along with the calls explains why they shouldn't be here:
> >
> > /* DDI buffer programming unnecessary during driver-load/resume
> >  * as it's already done during modeset initialization then.
> >  * It's also invalid here as encoder list is still uninitialized.
> >  */
> >
> > And the protection is not working well causing issues during the boot where
> > power well initialization doesn't go as expected.
> >
> > So this call during modeset initialization where the encoder list is yet
> > unitilized causes a NULL dereference breaking i915.
> >
> > While we don't find the right protection and we don't understand why this is
> > actually needed I believe it is safe to just remove these calls.
> >
> > Cc: Suketu Shah <suketu.j.shah@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> But if we shut down the power well the hw will forget these values. Where
> are they programmed now? Which commit broke this, how does this blow up?
> Please add more details, atm I have no idea what to do with this patch but
> be confused about it ...
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 8 --------
> >  1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 85c35fd..f7027ea 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -246,7 +246,6 @@ static void skl_power_well_post_enable(struct drm_i915_private *dev_priv,
> >       }
> >
> >       if (power_well->data == SKL_DISP_PW_1) {
> > -             intel_prepare_ddi(dev);
> >               gen8_irq_power_well_post_enable(dev_priv, 1 << PIPE_A);
> >       }
> >  }
> > @@ -632,13 +631,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >                               power_well->data == SKL_DISP_PW_2) {
> >                               if (SKL_ENABLE_DC6(dev)) {
> >                                       skl_disable_dc6(dev_priv);
> > -                                     /*
> > -                                      * DDI buffer programming unnecessary during driver-load/resume
> > -                                      * as it's already done during modeset initialization then.
> > -                                      * It's also invalid here as encoder list is still uninitialized.
> > -                                      */
> > -                                     if (!dev_priv->power_domains.initializing)
> > -                                             intel_prepare_ddi(dev);
> >                               } else {
> >                                       gen9_disable_dc5(dev_priv);
> >                               }
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx