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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
