From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/3] drm/i915: close PM interrupt masking races in the rps work func Date: Mon, 05 Sep 2011 13:15:17 +0100 Message-ID: References: <20110904084953.16cd10a2@bwidawsk.net> <1315150502-12537-1-git-send-email-daniel.vetter@ffwll.ch> <1315150502-12537-3-git-send-email-daniel.vetter@ffwll.ch> <20110904100817.16c6c4cc@bwidawsk.net> <20110904192648.GB2799@phenom.ffwll.local> <20110904195657.GB17304@cloud01> <20110904201030.GE2799@phenom.ffwll.local> <20110904213856.GA18071@cloud01> <20110905063807.GA2921@phenom.ffwll.local> <20110904235152.36175596@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 146189E936 for ; Mon, 5 Sep 2011 05:15:24 -0700 (PDT) In-Reply-To: <20110904235152.36175596@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky , Daniel Vetter Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sun, 4 Sep 2011 23:51:52 -0700, Ben Widawsky wrote: > On Mon, 5 Sep 2011 08:38:07 +0200 > Daniel Vetter wrote: > > > On Sun, Sep 04, 2011 at 09:38:56PM +0000, Ben Widawsky wrote: > > > Oops, you're totally right, I think I meant: > > > - I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); > > > + I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir); > > > > Imo still racy without the irqsafe rps_lock around it. gcc is free to > > compile that into a separate load and store which the irq handler can > > get in between and change dev_priv->pm_iir and PMIMR. The race is now > > only one instruction wide, though ;-) > > -Daniel > > You are absolutely correct. The modification to GEN6_PMIMR must be > within the protection of rps_lock. Right. However, I don't see why we need to hold the mutex though. Why are we touching PMIMR in gen6_enable_rps()? Afaics, it is because gen6_disable_rps() may leave a stale PMIMR (it sets dev_priv->pm_iir to 0, causing the existing work-handler to return early and not touch PMIMR). I believe everything is correct if we clear PMIMR on module load, remove the setting of PMIMR from gen6_enable_rps() and clear PMIMR under the rps lock at the top of the work handler (which both Daniel and I have desired to do... ;-) -Chris -- Chris Wilson, Intel Open Source Technology Centre