* [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt @ 2014-08-25 23:24 Jesse Barnes 2014-08-26 5:26 ` Oliver Hartkopp 2014-08-26 7:23 ` Daniel Vetter 0 siblings, 2 replies; 14+ messages in thread From: Jesse Barnes @ 2014-08-25 23:24 UTC (permalink / raw) To: intel-gfx; +Cc: socketcan This happens in irq_postinstall before we've set the pm._irqs_disabled flag, but shouldn't warn. So add a nowarn variant to allow this to happen w/o a backtrace and keep the rest of the IRQ tracking code happy. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------ 1 files changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d5445e7..ec1d9fe 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ /* For display hotplug interrupt */ static void +ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask) +{ + if ((dev_priv->irq_mask & mask) != 0) { + dev_priv->irq_mask &= ~mask; + I915_WRITE(DEIMR, dev_priv->irq_mask); + POSTING_READ(DEIMR); + } +} + +static void ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) { assert_spin_locked(&dev_priv->irq_lock); @@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) if (WARN_ON(!intel_irqs_enabled(dev_priv))) return; - if ((dev_priv->irq_mask & mask) != 0) { - dev_priv->irq_mask &= ~mask; - I915_WRITE(DEIMR, dev_priv->irq_mask); - POSTING_READ(DEIMR); - } + ironlake_enable_display_irq_nowarn(dev_priv, mask); } static void @@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) * setup is guaranteed to run in single-threaded context. But we * need it to make the assert_spin_locked happy. */ spin_lock_irqsave(&dev_priv->irq_lock, irqflags); - ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT); + ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-25 23:24 [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt Jesse Barnes @ 2014-08-26 5:26 ` Oliver Hartkopp 2014-08-26 7:23 ` Daniel Vetter 1 sibling, 0 replies; 14+ messages in thread From: Oliver Hartkopp @ 2014-08-26 5:26 UTC (permalink / raw) To: Jesse Barnes, intel-gfx On 26.08.2014 01:24, Jesse Barnes wrote: > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > a backtrace and keep the rest of the IRQ tracking code happy. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Reported-by: Oliver Hartkopp <socketcan@hartkopp.net> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> Yeah. That fixed it. Thanks Jesse! [ 2.900147] Linux agpgart interface v0.103 [ 2.900475] agpgart-intel 0000:00:00.0: Intel HD Graphics Chipset [ 2.900579] agpgart-intel 0000:00:00.0: detected gtt size: 2097152K total, 262144K mappable [ 2.901354] agpgart-intel 0000:00:00.0: detected 32768K stolen memory [ 2.901743] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0xe0000000 [ 2.901995] [drm] Initialized drm 1.1.0 20060810 [ 2.904212] [drm] Memory usable by graphics device = 2048M [ 2.904295] [drm] Replacing VGA console driver [ 2.904961] Console: switching to colour dummy device 80x25 [ 2.937950] i915 0000:00:02.0: irq 24 for MSI/MSI-X [ 2.937962] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 2.937965] [drm] Driver supports precise vblank timestamp query. [ 2.938310] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem [ 3.030687] fbcon: inteldrmfb (fb0) is primary device [ 3.494467] tsc: Refined TSC clocksource calibration: 2792.999 MHz [ 4.176644] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up [ 4.495814] Switched to clocksource tsc [ 4.529922] Console: switching to colour frame buffer device 240x67 [ 4.536702] i915 0000:00:02.0: fb0: inteldrmfb frame buffer device [ 4.536769] i915 0000:00:02.0: registered panic notifier [ 4.548333] ACPI: Video Device [VID2] (multi-head: yes rom: no post: no) [ 4.900233] acpi device:45: registered as cooling_device0 [ 4.924178] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:02/input/input4 [ 4.924371] [drm] Initialized i915 1.6.0 20140725 for 0000:00:02.0 on minor 0 Btw. do you know what [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up is about? Regards, Oliver > --- > drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------ > 1 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d5445e7..ec1d9fe 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ > > /* For display hotplug interrupt */ > static void > +ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask) > +{ > + if ((dev_priv->irq_mask & mask) != 0) { > + dev_priv->irq_mask &= ~mask; > + I915_WRITE(DEIMR, dev_priv->irq_mask); > + POSTING_READ(DEIMR); > + } > +} > + > +static void > ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) > { > assert_spin_locked(&dev_priv->irq_lock); > @@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) > if (WARN_ON(!intel_irqs_enabled(dev_priv))) > return; > > - if ((dev_priv->irq_mask & mask) != 0) { > - dev_priv->irq_mask &= ~mask; > - I915_WRITE(DEIMR, dev_priv->irq_mask); > - POSTING_READ(DEIMR); > - } > + ironlake_enable_display_irq_nowarn(dev_priv, mask); > } > > static void > @@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > * setup is guaranteed to run in single-threaded context. But we > * need it to make the assert_spin_locked happy. */ > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > - ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT); > + ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-25 23:24 [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt Jesse Barnes 2014-08-26 5:26 ` Oliver Hartkopp @ 2014-08-26 7:23 ` Daniel Vetter 2014-08-26 18:52 ` Jesse Barnes 1 sibling, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2014-08-26 7:23 UTC (permalink / raw) To: Jesse Barnes; +Cc: socketcan, intel-gfx On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > a backtrace and keep the rest of the IRQ tracking code happy. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c right above the drm_irq_install call? In intel_runtime_pm_restore_interrupts we also set it to false before we call the various hooks. Also the commit message is a bit thin on the usual details like which commits introduced this regression, so that maintainers know where to apply this to. -Daniel > --- > drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------ > 1 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d5445e7..ec1d9fe 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ > > /* For display hotplug interrupt */ > static void > +ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask) > +{ > + if ((dev_priv->irq_mask & mask) != 0) { > + dev_priv->irq_mask &= ~mask; > + I915_WRITE(DEIMR, dev_priv->irq_mask); > + POSTING_READ(DEIMR); > + } > +} > + > +static void > ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) > { > assert_spin_locked(&dev_priv->irq_lock); > @@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask) > if (WARN_ON(!intel_irqs_enabled(dev_priv))) > return; > > - if ((dev_priv->irq_mask & mask) != 0) { > - dev_priv->irq_mask &= ~mask; > - I915_WRITE(DEIMR, dev_priv->irq_mask); > - POSTING_READ(DEIMR); > - } > + ironlake_enable_display_irq_nowarn(dev_priv, mask); > } > > static void > @@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > * setup is guaranteed to run in single-threaded context. But we > * need it to make the assert_spin_locked happy. */ > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > - ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT); > + ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > -- > 1.7.5.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 7:23 ` Daniel Vetter @ 2014-08-26 18:52 ` Jesse Barnes 2014-08-26 19:03 ` Oliver Hartkopp 2014-08-26 20:51 ` Daniel Vetter 0 siblings, 2 replies; 14+ messages in thread From: Jesse Barnes @ 2014-08-26 18:52 UTC (permalink / raw) To: Daniel Vetter; +Cc: socketcan, intel-gfx On Tue, 26 Aug 2014 09:23:54 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > > a backtrace and keep the rest of the IRQ tracking code happy. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c > right above the drm_irq_install call? In > intel_runtime_pm_restore_interrupts we also set it to false before we call > the various hooks. I didn't check on all the paths whether that was safe, we have a lot of warnings. And really this init time thing is a special case, so it made sense to me. > Also the commit message is a bit thin on the usual details like which > commits introduced this regression, so that maintainers know where to > apply this to. I don't have the commit... Oliver do you have it handy? -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 18:52 ` Jesse Barnes @ 2014-08-26 19:03 ` Oliver Hartkopp 2014-08-26 19:10 ` Jesse Barnes 2014-08-26 20:51 ` Daniel Vetter 1 sibling, 1 reply; 14+ messages in thread From: Oliver Hartkopp @ 2014-08-26 19:03 UTC (permalink / raw) To: Jesse Barnes, Daniel Vetter; +Cc: intel-gfx On 26.08.2014 20:52, Jesse Barnes wrote: > On Tue, 26 Aug 2014 09:23:54 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: >>> This happens in irq_postinstall before we've set the pm._irqs_disabled flag, >>> but shouldn't warn. So add a nowarn variant to allow this to happen w/o >>> a backtrace and keep the rest of the IRQ tracking code happy. >>> >> Also the commit message is a bit thin on the usual details like which >> commits introduced this regression, so that maintainers know where to >> apply this to. > > I don't have the commit... Oliver do you have it handy? > Hm - I really can not tell what has been done to introduce this regression. I just saw the warning on my machine after upgrading to 3.17 ... You can ask me about linux/net/can/ but not the drm stuff ;-) Cheers, Oliver ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 19:03 ` Oliver Hartkopp @ 2014-08-26 19:10 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2014-08-26 19:10 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: intel-gfx On Tue, 26 Aug 2014 21:03:11 +0200 Oliver Hartkopp <socketcan@hartkopp.net> wrote: > On 26.08.2014 20:52, Jesse Barnes wrote: > > On Tue, 26 Aug 2014 09:23:54 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > >>> This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > >>> but shouldn't warn. So add a nowarn variant to allow this to happen w/o > >>> a backtrace and keep the rest of the IRQ tracking code happy. > >>> > > >> Also the commit message is a bit thin on the usual details like which > >> commits introduced this regression, so that maintainers know where to > >> apply this to. > > > > I don't have the commit... Oliver do you have it handy? > > > > Hm - I really can not tell what has been done to introduce this regression. > I just saw the warning on my machine after upgrading to 3.17 ... > > You can ask me about linux/net/can/ but not the drm stuff ;-) I think it was this one Daniel, or the combination of patches around it: commit 95f25beddba2ec9510b249740bacc11eca70cf75 Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Fri Jun 20 09:29:22 2014 -0700 drm/i915: set pm._irqs_disabled at IRQ init time -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 18:52 ` Jesse Barnes 2014-08-26 19:03 ` Oliver Hartkopp @ 2014-08-26 20:51 ` Daniel Vetter 2014-08-26 21:18 ` Jesse Barnes ` (2 more replies) 1 sibling, 3 replies; 14+ messages in thread From: Daniel Vetter @ 2014-08-26 20:51 UTC (permalink / raw) To: Jesse Barnes; +Cc: Oliver Hartkopp, intel-gfx On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 26 Aug 2014 09:23:54 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o >> > a backtrace and keep the rest of the IRQ tracking code happy. >> > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c >> right above the drm_irq_install call? In >> intel_runtime_pm_restore_interrupts we also set it to false before we call >> the various hooks. > > I didn't check on all the paths whether that was safe, we have a lot of > warnings. > > And really this init time thing is a special case, so it made sense to > me. Well I fully agree that your patch is the safe option and much easier to review, too. But driver load/resume are the most fragile paths we have in our codebase - you look at them and it falls apart. And we have absolutely no handle on these issues on a fundamental level at all (compared to other areas where we reworked the code or added enough tests to pretty much kill entire classes of regressions). The only half-assed thing we can do is try to not have too much complexity (so that you can still understand it, we're probably over that already) and lock down the ordering and other constraints with piles of really loud WARN_ON asserts. Your patch both removes WARN_ONs from these codepaths and adds special cases, so falls a bit short on those metrics. And if I'm not mistaken (like I've said the code is too complex by now to really understand) the below change should get us there, too. So I want to see whether that wouldn't work before going with your patch. Oliver, can you please test the below diff? Thanks, Daniel diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f19dbff0e73b..915a60b48159 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct drm_device *dev) intel_power_domains_init_hw(dev_priv); + /* + * We enable some interrupt sources in our postinstall hooks, so mark + * interrupts as enabled _before_ actually enabling them to avoid + * special cases in our ordering checks. + */ + dev_priv->pm._irqs_disabled = false; + ret = drm_irq_install(dev, dev->pdev->irq); if (ret) goto cleanup_gem_stolen; - dev_priv->pm._irqs_disabled = false; - /* Important: The output setup functions called by modeset_init need * working irqs for e.g. gmbus and dp aux transfers. */ intel_modeset_init(dev); -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 20:51 ` Daniel Vetter @ 2014-08-26 21:18 ` Jesse Barnes 2014-08-26 21:33 ` Daniel Vetter 2014-08-27 6:30 ` Oliver Hartkopp 2014-08-27 19:59 ` Jesse Barnes 2 siblings, 1 reply; 14+ messages in thread From: Jesse Barnes @ 2014-08-26 21:18 UTC (permalink / raw) To: Daniel Vetter; +Cc: Oliver Hartkopp, intel-gfx On Tue, 26 Aug 2014 22:51:13 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Tue, 26 Aug 2014 09:23:54 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > >> > a backtrace and keep the rest of the IRQ tracking code happy. > >> > > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> > >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c > >> right above the drm_irq_install call? In > >> intel_runtime_pm_restore_interrupts we also set it to false before we call > >> the various hooks. > > > > I didn't check on all the paths whether that was safe, we have a lot of > > warnings. > > > > And really this init time thing is a special case, so it made sense to > > me. > > Well I fully agree that your patch is the safe option and much easier to > review, too. > > But driver load/resume are the most fragile paths we have in our codebase > - you look at them and it falls apart. And we have absolutely no handle on > these issues on a fundamental level at all (compared to other areas where > we reworked the code or added enough tests to pretty much kill entire > classes of regressions). The only half-assed thing we can do is try to not > have too much complexity (so that you can still understand it, we're > probably over that already) and lock down the ordering and other > constraints with piles of really loud WARN_ON asserts. > > Your patch both removes WARN_ONs from these codepaths and adds special > cases, so falls a bit short on those metrics. And if I'm not mistaken > (like I've said the code is too complex by now to really understand) the > below change should get us there, too. So I want to see whether that > wouldn't work before going with your patch. Yes, it might work, but if you look through the history, we set this field carefully; first to true in the irq_init code, then to false only after the irq_install completes. So I think your fragility arguments apply to this change too. -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 21:18 ` Jesse Barnes @ 2014-08-26 21:33 ` Daniel Vetter 0 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2014-08-26 21:33 UTC (permalink / raw) To: Jesse Barnes; +Cc: Oliver Hartkopp, intel-gfx On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Daniel Vetter <daniel@ffwll.ch> wrote: > >> >> On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > On Tue, 26 Aug 2014 09:23:54 +0200 >> > Daniel Vetter <daniel@ffwll.ch> wrote: >> > >> >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: >> >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, >> >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o >> >> > a backtrace and keep the rest of the IRQ tracking code happy. >> >> > >> >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> >> >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c >> >> right above the drm_irq_install call? In >> >> intel_runtime_pm_restore_interrupts we also set it to false before we call >> >> the various hooks. >> > >> > I didn't check on all the paths whether that was safe, we have a lot of >> > warnings. >> > >> > And really this init time thing is a special case, so it made sense to >> > me. >> >> Well I fully agree that your patch is the safe option and much easier to >> review, too. >> >> But driver load/resume are the most fragile paths we have in our codebase >> - you look at them and it falls apart. And we have absolutely no handle on >> these issues on a fundamental level at all (compared to other areas where >> we reworked the code or added enough tests to pretty much kill entire >> classes of regressions). The only half-assed thing we can do is try to not >> have too much complexity (so that you can still understand it, we're >> probably over that already) and lock down the ordering and other >> constraints with piles of really loud WARN_ON asserts. >> >> Your patch both removes WARN_ONs from these codepaths and adds special >> cases, so falls a bit short on those metrics. And if I'm not mistaken >> (like I've said the code is too complex by now to really understand) the >> below change should get us there, too. So I want to see whether that >> wouldn't work before going with your patch. > > Yes, it might work, but if you look through the history, we set this > field carefully; first to true in the irq_init code, then to false only > after the irq_install completes. So I think your fragility arguments > apply to this change too. Well we've done it in 4 commits or so, but currently we have: - Set irqs_disabled to true early in driver load to make sure checks that. That's done in irq_init, which is totally not the function that enables interrupts, only the function that initializes all the vtables and similar things. We actually have a fairly sane naming scheme nowadays (not fully consistent ofc): _init is sw setup, _enable/_hw_init is the actual hw setup. That is done in 95f25beddba2ec9510b249740bacc11eca70cf75 - Set irqs_disabled to false right after the irqs are actually enabled. This is done in ed2e6df18935beb3d63613c50103bf9757b2aa85 So my change should only move the flag change over the ->preinstall and ->postinstall hooks. I've done a little audit and didn't spot anything amiss. Furthermore the runtime pm setup already clears irqs_disabled _before_ calling these two hooks. So I think overall this is solid. And now I've also written the commit message for the patch if it tests out on Oliver's box ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 20:51 ` Daniel Vetter 2014-08-26 21:18 ` Jesse Barnes @ 2014-08-27 6:30 ` Oliver Hartkopp 2014-08-27 19:59 ` Jesse Barnes 2 siblings, 0 replies; 14+ messages in thread From: Oliver Hartkopp @ 2014-08-27 6:30 UTC (permalink / raw) To: Daniel Vetter, Jesse Barnes; +Cc: intel-gfx On 26.08.2014 22:51, Daniel Vetter wrote: > > > Oliver, can you please test the below diff? > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index f19dbff0e73b..915a60b48159 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct drm_device *dev) > > intel_power_domains_init_hw(dev_priv); > > + /* > + * We enable some interrupt sources in our postinstall hooks, so mark > + * interrupts as enabled _before_ actually enabling them to avoid > + * special cases in our ordering checks. > + */ > + dev_priv->pm._irqs_disabled = false; > + > ret = drm_irq_install(dev, dev->pdev->irq); > if (ret) > goto cleanup_gem_stolen; > > - dev_priv->pm._irqs_disabled = false; > - > /* Important: The output setup functions called by modeset_init need > * working irqs for e.g. gmbus and dp aux transfers. */ > intel_modeset_init(dev); > Yes - that one works too! (see below) Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> Thanks, Oliver [ 2.894351] Linux agpgart interface v0.103 [ 2.894680] agpgart-intel 0000:00:00.0: Intel HD Graphics Chipset [ 2.894783] agpgart-intel 0000:00:00.0: detected gtt size: 2097152K total, 262144K mappable [ 2.895558] agpgart-intel 0000:00:00.0: detected 32768K stolen memory [ 2.895986] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0xe0000000 [ 2.896248] [drm] Initialized drm 1.1.0 20060810 [ 2.898458] [drm] Memory usable by graphics device = 2048M [ 2.898541] [drm] Replacing VGA console driver [ 2.899207] Console: switching to colour dummy device 80x25 [ 2.931603] i915 0000:00:02.0: irq 24 for MSI/MSI-X [ 2.931615] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 2.931618] [drm] Driver supports precise vblank timestamp query. [ 2.931964] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem [ 3.023215] fbcon: inteldrmfb (fb0) is primary device [ 3.484082] tsc: Refined TSC clocksource calibration: 2792.999 MHz [ 4.170441] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up [ 4.485295] Switched to clocksource tsc [ 4.533314] Console: switching to colour frame buffer device 240x67 [ 4.550357] i915 0000:00:02.0: fb0: inteldrmfb frame buffer device [ 4.550446] i915 0000:00:02.0: registered panic notifier [ 4.561636] ACPI: Video Device [VID2] (multi-head: yes rom: no post: no) [ 4.913748] acpi device:45: registered as cooling_device0 [ 4.941732] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:02/input/input4 [ 4.941822] [drm] Initialized i915 1.6.0 20140725 for 0000:00:02.0 on minor 0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-26 20:51 ` Daniel Vetter 2014-08-26 21:18 ` Jesse Barnes 2014-08-27 6:30 ` Oliver Hartkopp @ 2014-08-27 19:59 ` Jesse Barnes 2014-08-27 21:33 ` Daniel Vetter 2014-08-29 7:08 ` Sun, Yi 2 siblings, 2 replies; 14+ messages in thread From: Jesse Barnes @ 2014-08-27 19:59 UTC (permalink / raw) To: Daniel Vetter; +Cc: Oliver Hartkopp, intel-gfx Yi, can you get this one run through testing on multiple platforms? We just want to make sure there's not some path we missed that's gonna spew a warning with this change. Thanks, Jesse On Tue, 26 Aug 2014 22:51:13 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Tue, 26 Aug 2014 09:23:54 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > >> > a backtrace and keep the rest of the IRQ tracking code happy. > >> > > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> > >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c > >> right above the drm_irq_install call? In > >> intel_runtime_pm_restore_interrupts we also set it to false before we call > >> the various hooks. > > > > I didn't check on all the paths whether that was safe, we have a lot of > > warnings. > > > > And really this init time thing is a special case, so it made sense to > > me. > > Well I fully agree that your patch is the safe option and much easier to > review, too. > > But driver load/resume are the most fragile paths we have in our codebase > - you look at them and it falls apart. And we have absolutely no handle on > these issues on a fundamental level at all (compared to other areas where > we reworked the code or added enough tests to pretty much kill entire > classes of regressions). The only half-assed thing we can do is try to not > have too much complexity (so that you can still understand it, we're > probably over that already) and lock down the ordering and other > constraints with piles of really loud WARN_ON asserts. > > Your patch both removes WARN_ONs from these codepaths and adds special > cases, so falls a bit short on those metrics. And if I'm not mistaken > (like I've said the code is too complex by now to really understand) the > below change should get us there, too. So I want to see whether that > wouldn't work before going with your patch. > > Oliver, can you please test the below diff? > > Thanks, Daniel > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index f19dbff0e73b..915a60b48159 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct drm_device *dev) > > intel_power_domains_init_hw(dev_priv); > > + /* > + * We enable some interrupt sources in our postinstall hooks, so mark > + * interrupts as enabled _before_ actually enabling them to avoid > + * special cases in our ordering checks. > + */ > + dev_priv->pm._irqs_disabled = false; > + > ret = drm_irq_install(dev, dev->pdev->irq); > if (ret) > goto cleanup_gem_stolen; > > - dev_priv->pm._irqs_disabled = false; > - > /* Important: The output setup functions called by modeset_init need > * working irqs for e.g. gmbus and dp aux transfers. */ > intel_modeset_init(dev); -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-27 19:59 ` Jesse Barnes @ 2014-08-27 21:33 ` Daniel Vetter 2014-08-27 21:36 ` Jesse Barnes 2014-08-29 7:08 ` Sun, Yi 1 sibling, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2014-08-27 21:33 UTC (permalink / raw) To: Jesse Barnes; +Cc: Oliver Hartkopp, intel-gfx On Wed, Aug 27, 2014 at 9:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Yi, can you get this one run through testing on multiple platforms? We > just want to make sure there's not some path we missed that's gonna > spew a warning with this change. I think that amount of testing is totally out of balance to keep a rather tiny warninig alive in the driver load code. The major use for the pm._irqs_disabled is to catch runtime pm bugs where code uses changes irq settings while the chip is off. That part will still be 100% effective with Jesse's patch. So I recommended that we instead merge Jesses patch instead of mine for -fixes. Jesse's patch has my r-b fwiw. -Daniel > > Thanks, > Jesse > > On Tue, 26 Aug 2014 22:51:13 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> >> On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > On Tue, 26 Aug 2014 09:23:54 +0200 >> > Daniel Vetter <daniel@ffwll.ch> wrote: >> > >> >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: >> >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, >> >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o >> >> > a backtrace and keep the rest of the IRQ tracking code happy. >> >> > >> >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> >> >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c >> >> right above the drm_irq_install call? In >> >> intel_runtime_pm_restore_interrupts we also set it to false before we call >> >> the various hooks. >> > >> > I didn't check on all the paths whether that was safe, we have a lot of >> > warnings. >> > >> > And really this init time thing is a special case, so it made sense to >> > me. >> >> Well I fully agree that your patch is the safe option and much easier to >> review, too. >> >> But driver load/resume are the most fragile paths we have in our codebase >> - you look at them and it falls apart. And we have absolutely no handle on >> these issues on a fundamental level at all (compared to other areas where >> we reworked the code or added enough tests to pretty much kill entire >> classes of regressions). The only half-assed thing we can do is try to not >> have too much complexity (so that you can still understand it, we're >> probably over that already) and lock down the ordering and other >> constraints with piles of really loud WARN_ON asserts. >> >> Your patch both removes WARN_ONs from these codepaths and adds special >> cases, so falls a bit short on those metrics. And if I'm not mistaken >> (like I've said the code is too complex by now to really understand) the >> below change should get us there, too. So I want to see whether that >> wouldn't work before going with your patch. >> >> Oliver, can you please test the below diff? >> >> Thanks, Daniel >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index f19dbff0e73b..915a60b48159 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct drm_device *dev) >> >> intel_power_domains_init_hw(dev_priv); >> >> + /* >> + * We enable some interrupt sources in our postinstall hooks, so mark >> + * interrupts as enabled _before_ actually enabling them to avoid >> + * special cases in our ordering checks. >> + */ >> + dev_priv->pm._irqs_disabled = false; >> + >> ret = drm_irq_install(dev, dev->pdev->irq); >> if (ret) >> goto cleanup_gem_stolen; >> >> - dev_priv->pm._irqs_disabled = false; >> - >> /* Important: The output setup functions called by modeset_init need >> * working irqs for e.g. gmbus and dp aux transfers. */ >> intel_modeset_init(dev); > > > -- > Jesse Barnes, Intel Open Source Technology Center -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-27 21:33 ` Daniel Vetter @ 2014-08-27 21:36 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2014-08-27 21:36 UTC (permalink / raw) To: Daniel Vetter; +Cc: Oliver Hartkopp, intel-gfx On Wed, 27 Aug 2014 23:33:05 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Aug 27, 2014 at 9:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > Yi, can you get this one run through testing on multiple platforms? We > > just want to make sure there's not some path we missed that's gonna > > spew a warning with this change. > > I think that amount of testing is totally out of balance to keep a > rather tiny warninig alive in the driver load code. The major use for > the pm._irqs_disabled is to catch runtime pm bugs where code uses > changes irq settings while the chip is off. That part will still be > 100% effective with Jesse's patch. > > So I recommended that we instead merge Jesses patch instead of mine > for -fixes. Jesse's patch has my r-b fwiw. Either is fine with me. Maybe you could cc the PRTS and see if it gives you results! -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt 2014-08-27 19:59 ` Jesse Barnes 2014-08-27 21:33 ` Daniel Vetter @ 2014-08-29 7:08 ` Sun, Yi 1 sibling, 0 replies; 14+ messages in thread From: Sun, Yi @ 2014-08-29 7:08 UTC (permalink / raw) To: Jesse Barnes, Daniel Vetter; +Cc: Oliver Hartkopp, intel-gfx, Liu, Lei A I have tested both Jesse and Daniel's patch. On IVB both two will introduce a new warning. On HSW Daniel's patch can cause black screen. On BDW both two patch can cause black screen. _________________________________________________________________________________________________________________ | | Bad commit | Jesse's patch | Dainel's patch | |IVB | irq WARN on boot | No irq warning, but a new one 'WARNING! power/level is deprecated; use power/control instead' | |HSW | irq WARN on boot | No irq warning | black screen | |SNB | irq WARN on boot | No irq warning | No irq warning | |BDW | irq warn on boot | black screen | black screen | |___________|___________________|____________________________________________________|____________________________| Thanks --Sun, Yi > -----Original Message----- > From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org] > Sent: Thursday, August 28, 2014 3:59 AM > To: Daniel Vetter > Cc: intel-gfx; Oliver Hartkopp; Sun, Yi > Subject: Re: [Intel-gfx] [PATCH] drm/i915/ilk: special case enabling of > PCU_EVENT interrupt > > Yi, can you get this one run through testing on multiple platforms? We just > want to make sure there's not some path we missed that's gonna spew a > warning with this change. > > Thanks, > Jesse > > On Tue, 26 Aug 2014 22:51:13 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> > wrote: > > > On Tue, 26 Aug 2014 09:23:54 +0200 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > > >> > This happens in irq_postinstall before we've set the > > >> > pm._irqs_disabled flag, but shouldn't warn. So add a nowarn > > >> > variant to allow this to happen w/o a backtrace and keep the rest of the > IRQ tracking code happy. > > >> > > > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > >> > > >> Shouldn't we instead just move the pm._irqs_disabled = false in > > >> i915_dma.c right above the drm_irq_install call? In > > >> intel_runtime_pm_restore_interrupts we also set it to false before > > >> we call the various hooks. > > > > > > I didn't check on all the paths whether that was safe, we have a lot > > > of warnings. > > > > > > And really this init time thing is a special case, so it made sense > > > to me. > > > > Well I fully agree that your patch is the safe option and much easier > > to review, too. > > > > But driver load/resume are the most fragile paths we have in our > > codebase > > - you look at them and it falls apart. And we have absolutely no > > handle on these issues on a fundamental level at all (compared to > > other areas where we reworked the code or added enough tests to pretty > > much kill entire classes of regressions). The only half-assed thing we > > can do is try to not have too much complexity (so that you can still > > understand it, we're probably over that already) and lock down the > > ordering and other constraints with piles of really loud WARN_ON asserts. > > > > Your patch both removes WARN_ONs from these codepaths and adds special > > cases, so falls a bit short on those metrics. And if I'm not mistaken > > (like I've said the code is too complex by now to really understand) > > the below change should get us there, too. So I want to see whether > > that wouldn't work before going with your patch. > > > > Oliver, can you please test the below diff? > > > > Thanks, Daniel > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > b/drivers/gpu/drm/i915/i915_dma.c index f19dbff0e73b..915a60b48159 > > 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct > > drm_device *dev) > > > > intel_power_domains_init_hw(dev_priv); > > > > + /* > > + * We enable some interrupt sources in our postinstall hooks, so mark > > + * interrupts as enabled _before_ actually enabling them to avoid > > + * special cases in our ordering checks. > > + */ > > + dev_priv->pm._irqs_disabled = false; > > + > > ret = drm_irq_install(dev, dev->pdev->irq); > > if (ret) > > goto cleanup_gem_stolen; > > > > - dev_priv->pm._irqs_disabled = false; > > - > > /* Important: The output setup functions called by modeset_init need > > * working irqs for e.g. gmbus and dp aux transfers. */ > > intel_modeset_init(dev); > > > -- > Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-08-29 7:08 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-25 23:24 [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt Jesse Barnes 2014-08-26 5:26 ` Oliver Hartkopp 2014-08-26 7:23 ` Daniel Vetter 2014-08-26 18:52 ` Jesse Barnes 2014-08-26 19:03 ` Oliver Hartkopp 2014-08-26 19:10 ` Jesse Barnes 2014-08-26 20:51 ` Daniel Vetter 2014-08-26 21:18 ` Jesse Barnes 2014-08-26 21:33 ` Daniel Vetter 2014-08-27 6:30 ` Oliver Hartkopp 2014-08-27 19:59 ` Jesse Barnes 2014-08-27 21:33 ` Daniel Vetter 2014-08-27 21:36 ` Jesse Barnes 2014-08-29 7:08 ` Sun, Yi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.