From: Chris Wilson <chris@chris-wilson.co.uk>
To: Ben Widawsky <ben@bwidawsk.net>, Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, intel-gfx@lists.freedesktop.org
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 [thread overview]
Message-ID: <e39f63$1eoi28@fmsmga002.fm.intel.com> (raw)
In-Reply-To: <20110904235152.36175596@bwidawsk.net>
On Sun, 4 Sep 2011 23:51:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, 5 Sep 2011 08:38:07 +0200
> Daniel Vetter <daniel@ffwll.ch> 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
next prev parent reply other threads:[~2011-09-05 12:15 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-04 3:24 [PATCH] drm/i915: Fix rps irq warning Ben Widawsky
2011-09-04 9:03 ` Chris Wilson
2011-09-04 15:49 ` Ben Widawsky
2011-09-04 15:34 ` [PATCH 0/3] slaughter rps races some more Daniel Vetter
2011-09-04 15:35 ` [PATCH 1/3] drm/i915: close PM interrupt masking races in the irq handler Daniel Vetter
2011-09-04 17:09 ` Ben Widawsky
2011-09-04 15:35 ` [PATCH 2/3] drm/i915: close PM interrupt masking races in the rps work func Daniel Vetter
2011-09-04 17:08 ` Ben Widawsky
2011-09-04 19:26 ` Daniel Vetter
2011-09-04 19:56 ` Ben Widawsky
2011-09-04 20:10 ` Daniel Vetter
2011-09-04 21:38 ` Ben Widawsky
2011-09-05 6:38 ` Daniel Vetter
2011-09-05 6:51 ` Ben Widawsky
2011-09-05 12:15 ` Chris Wilson [this message]
2011-09-04 15:35 ` [PATCH 3/3] drm/i915: close rps work vs. rps disable races Daniel Vetter
2011-09-04 17:23 ` Ben Widawsky
2011-09-04 19:17 ` Daniel Vetter
2011-09-04 19:50 ` Ben Widawsky
2011-09-04 19:57 ` Daniel Vetter
2011-09-05 8:15 ` [PATCH] drm/i915: properly cancel rps_work on module unload Daniel Vetter
2011-09-05 17:27 ` Ben Widawsky
-- strict thread matches above, loose matches on Subject: below --
2011-09-08 12:00 [PATCH 1/3] drm/i915: close PM interrupt masking races in the irq handler Daniel Vetter
2011-09-08 12:00 ` [PATCH 2/3] drm/i915: close PM interrupt masking races in the rps work func Daniel Vetter
2011-09-08 15:19 ` Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='e39f63$1eoi28@fmsmga002.fm.intel.com' \
--to=chris@chris-wilson.co.uk \
--cc=ben@bwidawsk.net \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.