Re: [RFC 1/4] drm/i915: split out uncore_mmio_debug

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [RFC 1/4] drm/i915: split out uncore_mmio_debug
Date: Wed, 26 Jun 2019 18:58:18 +0100	[thread overview]
Message-ID: <156157189800.26855.15905781510534161945@skylake-alporthouse-com> (raw)
In-Reply-To: <35062553-0982-376a-3b4a-4dcf19524c12@intel.com>

Quoting Daniele Ceraolo Spurio (2019-06-26 18:38:45)
> 
> 
> On 6/26/19 3:02 AM, Chris Wilson wrote:
> > Quoting Daniele Ceraolo Spurio (2019-06-24 21:31:49)
> >> @@ -605,18 +614,20 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
> >>   void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
> >>   {
> >>          spin_lock_irq(&uncore->lock);
> >> +       spin_lock(&uncore->debug->lock);
> >>          if (!uncore->user_forcewake.count++) {
> > 
> > Afaict, uncore->user_forcewake.count is only guarded by uncore->lock
> > and we only need to take debug->lock for the debug->unclaimed_mmio_check
> > manipulation. But there needs to be a shared usage counter around the
> > debug as it is shared state.
> > 
> >>                  intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
> >>   
> >>                  /* Save and disable mmio debugging for the user bypass */
> >>                  uncore->user_forcewake.saved_mmio_check =
> >> -                       uncore->unclaimed_mmio_check;
> >> +                       uncore->debug->unclaimed_mmio_check;
> >>                  uncore->user_forcewake.saved_mmio_debug =
> >>                          i915_modparams.mmio_debug;
> > 
> > Something more like
> > 
> > spin_lock_irq(&uncore->lock);
> > if (!uncore->user_forcewake_count++) {
> >       spin_lock(&uncore->debug->lock);
> >       if (!uncore->debug->usage_count++) {
> >               ...
> >       }
> >       spin_unlock(&uncore->debug->lock);
> > }
> > ...
> > spin_unlock_irq(&uncore->lock);
> > ?
> > -Chris
> > 
> 
> I do something similar in the next patch in the series (with the lock 
> still on the outside of the if). I've split that out because I've added 
> some functional changes to the flow. I can squash the 2 patches if you 
> thing it is better that way.

Yes. Looking at the second patch, that is clearer wrt what data we are
guarding with the locks.

Don't drop the intel_ prefix from intel_uncore_debug as it looks to
still be visible outside of its enclave (it's getting long, but it should
be more or less internal to the various intel_uncore implementations) and
squash these 2 as I feel more comfortable with intel_uncore_debug taking
control of the locking as required for sharing and reviewing the locking
wrt to the shared data.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx