From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915: Fix irq enable tracking in driver load Date: Thu, 04 Sep 2014 14:13:04 +0300 Message-ID: <87lhpzfxkv.fsf@intel.com> References: <1409127094-5843-1-git-send-email-daniel.vetter@ffwll.ch> <87oauvfxmd.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 23A896E2C7 for ; Thu, 4 Sep 2014 04:13:08 -0700 (PDT) In-Reply-To: <87oauvfxmd.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Intel Graphics Development Cc: Daniel Vetter , Oliver Hartkopp List-Id: intel-gfx@lists.freedesktop.org On Thu, 04 Sep 2014, Jani Nikula wrote: > On Wed, 27 Aug 2014, Daniel Vetter 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@SHSMSX101.ccr.corp.intel.com > > BR, > Jani. > > >> >> On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes 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 >> Date: Fri Jun 20 09:39:36 2014 -0700 >> >> drm/i915: clear pm._irqs_disabled field after installing IRQs >> >> Cc: Jesse Barnes >> Cc: Oliver Hartkopp >> Tested-by: Oliver Hartkopp >> Signed-off-by: Daniel Vetter >> --- >> 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