On Thu, 04 Sep 2014, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Wed, 27 Aug 2014, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: >> A bunch of warnings fire on some ->irq_postinstall hooks since those >> can enable interrupts (e.g. rps interrupts). And then our ordering >> self-checks fire and complain. >> >> To fix that set the tracking boolen before enabling the irqs witho >> drm_irq_install. Quoting the discussion with Jesse why that's safe: > > Yi Sun's testing result needs to be addressed one way or another before > merging this: ...before merging this *or* Jesse's patch, I mean. > > http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@xxxxxxxxxxxxxxxxxxxxxxxxxxxx > > BR, > Jani. > > >> >> On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote: >>> 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. >> >> This regression has been introduced in >> >> commit ed2e6df18935beb3d63613c50103bf9757b2aa85 >> Author: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> >> Date: Fri Jun 20 09:39:36 2014 -0700 >> >> drm/i915: clear pm._irqs_disabled field after installing IRQs >> >> Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> >> Cc: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> >> Tested-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> >> --- >> drivers/gpu/drm/i915/i915_dma.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index 3b24e3bb2106..498980661b2f 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); >> -- >> 2.0.1 >> > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
