[Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm

[Chris Wilson]

Quoting John Harrison (2019-01-10 01:10:09)
> On 1/9/2019 16:24, John Harrison wrote:
> > On 1/7/2019 03:54, Chris Wilson wrote:
> >> Frequently, we use intel_runtime_pm_get/_put around a small block.
> >> Formalise that usage by providing a macro to define such a block with an
> >> automatic closure to scope the intel_runtime_pm wakeref to that block,
> >> i.e. macro abuse smelling of python.
> >>
> >> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>   +#define with_intel_runtime_pm(i915, wf) \
> >> +    for (wf = intel_runtime_pm_get(i915); wf; \
> >> +         intel_runtime_pm_put(i915, wf), wf = 0)
> >> +
> >> +#define with_intel_runtime_pm_if_in_use(i915, wf) \
> >> +    for (wf = intel_runtime_pm_get_if_in_use(i915); wf; \
> >> +         intel_runtime_pm_put(i915, wf), wf = 0)
> >> +
> > This is a potential change in behaviour. Previously the simple 'get' 
> > version would unconditionally execute the wrapped code. Whereas now, 
> > if the get function fails for some reason and returns zero, the 
> > wrapped code will be skipped. Currently, the get() function can't 
> > return zero - it returns -1 in the case of the tracking code failing 
> > to allocate or similar. But is that guaranteed to be the case 
> > forevermore? It would be a better match for the original behaviour if 
> > the 'for' loop of the 'get' version was unconditional and only the 
> > 'get_if_in_use' version could skip. E.g. something like:
> >    for (intel_wakeref_t loop = -1, wf = intel_runtime_pm_get(i915) ; 
> > loop; intel_runtime_pm_put(i915, wf), wf = loop = 0)
> >
> > Although that does mean the wf becomes local to the loop. On the other 
> > hand, I'm also not sure why it needs to be external anyway? If it is 
> > guaranteed to be zero on exit and any value on entry is overwritten, 
> > then why have it external at all? Would it not be neater/smaller 
> > source to get rid of all the local instantiations?
> >
> > John.
> >
> Doh. Not sure why I was thinking C99 extensions were valid in the 
> kernel. I can't think of an alternative way to fix the above issues 
> without making the macro truly hideous. So maybe it's not enough of a 
> worry to worry about.

Using C99 would be a nice improvement for a lot of our macros, and I
hope it comes to pass.

Yes, the whole reason we return -1 on tracking-failure-but-rpm-success
is so that we keep 0 as meaning rpm-failure so that the different cases
are identifiable required for the markup and cookie tracking. So using
-1 here just falls out of the general case.
-Chris