Re: [Intel-gfx] [PATCH 9/9] drm/i915: Make intel_display_power_put_unchecked() an internal-only function

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Make intel_display_power_put_unchecked() an internal-only function
Date: Tue, 01 Dec 2020 11:13:38 +0000	[thread overview]
Message-ID: <160682121868.17248.514046911489151009@build.alporthouse.com> (raw)
In-Reply-To: <20201201105222.GA2849269@ideak-desk.fi.intel.com>

Quoting Imre Deak (2020-12-01 10:52:22)
> On Tue, Dec 01, 2020 at 08:14:40AM +0000, Chris Wilson wrote:
> > Quoting Imre Deak (2020-11-30 22:47:08)
> > > On Mon, Nov 30, 2020 at 10:07:01PM +0000, Chris Wilson wrote:
> > > > Quoting Imre Deak (2020-11-30 21:22:00)
> > > > > All the display power domain references are wakeref tracked now, so we
> > > > > can mark intel_display_power_put_unchecked() as an internal function
> > > > > (for suppressing wakeref tracking in non-debug builds).
> > > > > 
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > Ok, after all previous patches it will only be called from the header
> > > > after throwing away the wakeref.
> > > > 
> > > > I have a sneaky suspicion you might take another path after reviewing
> > > > the danger caused by the debug build, 
> > > 
> > > Yes, how about also adding:
> > > 
> > > +static inline void
> > > +____intel_display_power_put(struct drm_i915_private *i915,
> > > +                           enum intel_display_power_domain domain,
> > > +                           intel_wakeref_t wakeref)
> > > +{
> > > +       intel_display_power_put_unchecked(i915, domain);
> > > +}
> > > +
> > >  static inline void
> > >  intel_display_power_put(struct drm_i915_private *i915,
> > >                         enum intel_display_power_domain domain,
> > >                         intel_wakeref_t wakeref)
> > >  {
> > > -       intel_display_power_put_unchecked(i915, domain);
> > > +       ____intel_display_power_put(i915, domain, wakeref);
> > >  }
> > > 
> > > (and similar change for intel_display_power_put_async()) ?
> > 
> > Hmm. The compiler shouldn't DCE the wakeref since it has a side-effect.
> > We can but see.
> 
> Yes, arguments passed to functions are evaluated exactly once. The above
> extra call doesn't make sense anyway.
> 
> Are you ok with patch 4 then?

If you've done a quick non-debug test run, then I'm convinced I was
barking up the wrong tree.

The remaining patches are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
(since they were all blocked by the uncertainty in behaviour of the
fetch_and_zero).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx