Intel-GFX Archive on lore.kernel.org
 help / color / Atom feed
From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/i915: close rps work vs. rps disable races
Date: Sun, 4 Sep 2011 19:50:08 +0000
Message-ID: <20110904195008.GA17304@cloud01> (raw)
In-Reply-To: <20110904191723.GA2799@phenom.ffwll.local>

On Sun, Sep 04, 2011 at 09:17:24PM +0200, Daniel Vetter wrote:
> > >  	spin_lock_irq(&dev_priv->rps_lock);
> > >  	dev_priv->pm_iir = 0;
> > > +	I915_WRITE(GEN6_PMIER, 0);
> > >  	spin_unlock_irq(&dev_priv->rps_lock);
> > >  
> > >  	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > I'm not sure this actually fixes a problem. The existing code:
> > 1. disables all interrupts (no more can occur).
> > 2. sets pm_iir to 0 safe in rps lock
> > <workqueues can run at this point, but IMR has no effect with IER = 0>
> > 
> > I think you should do the cancel work sync somewhere in the code before
> > module unload (to be correct). I just don't think this fixes a race.
> 
> Well, we definitely need the cancel_work_sync in the unload path
> somewhere. I prefer it in the rps disable function. In the context of the
> other patches my patch description is a bit lousy because I've really only
> wanted to fix the unload race (and the PMIMR inconsistency) with this
> patch.
> 
> -Daniel

I still fail to see the inconsitency. The value of PMR no longer matters once
IER is cleared. What the wq or irq handler does after this point should
be fine so long as everything is setup up properly at enable time. This
is a minor detail.

So I would rework some of your comments a bit, and I also think it's
about time we create a central place to cancel workqueue items. Mostly
because I think this patch is subject to a deadlock with struct_mutex
(you're holding it when you call gen6_disable_rps(), but you're doing
cancel_work_sync on a workqueue which attempts to acquire struct_mutex.

Ben

  reply index

Thread overview: 22+ 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
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 [this message]
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

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=20110904195008.GA17304@cloud01 \
    --to=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

Intel-GFX Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/intel-gfx/0 intel-gfx/git/0.git
	git clone --mirror https://lore.kernel.org/intel-gfx/1 intel-gfx/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 intel-gfx intel-gfx/ https://lore.kernel.org/intel-gfx \
		intel-gfx@lists.freedesktop.org
	public-inbox-index intel-gfx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.intel-gfx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git