Re: [PATCH 09/39] drm/i915: Markup paired operations on wakerefs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Jani Nikula (2019-01-03 09:38:56)
> On Wed, 02 Jan 2019, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > The majority of runtime-pm operations are bounded and scoped within a
> > function; these are easy to verify that the wakeref are handled
> > correctly. We can employ the compiler to help us, and reduce the number
> > of wakerefs tracked when debugging, by passing around cookies provided
> > by the various rpm_get functions to their rpm_put counterpart. This
> > makes the pairing explicit, and given the required wakeref cookie the
> > compiler can verify that we pass an initialised value to the rpm_put
> > (quite handy for double checking error paths).
> 
> What a monster patch! :o Can't say I reviewed it all, but ack on the
> approach.
> 
> The series could use a cover letter... seems like this should be chopped
> up to smaller series perhaps. Or too many conflicts that way?

It had a cover letter about June when it was sent separately, since then
I've depended upon the bug fixes.
 
> Some minor nits inline.
> 
> BR,
> Jani.
> 
> >
> > For regular builds, the compiler should be able to eliminate the unused
> > local variables and the program growth should be minimal. Fwiw, it came
> > out as a net improvement as gcc was able to refactor rpm_get and
> > rpm_get_if_in_use together,
> >
> > add/remove: 1/1 grow/shrink: 20/9 up/down: 191/-268 (-77)
> > Function                                     old     new   delta
> > intel_runtime_pm_put_unchecked                 -     136    +136
> > i915_gem_unpark                              396     406     +10
> > intel_runtime_pm_get                         135     141      +6
> > intel_runtime_pm_get_noresume                136     141      +5
> > i915_perf_open_ioctl                        4375    4379      +4
> > i915_gpu_busy                                 72      76      +4
> > i915_gem_idle_work_handler                   954     958      +4
> > capture                                     6814    6818      +4
> > mock_gem_device                             1433    1436      +3
> > __execlists_submission_tasklet              2573    2576      +3
> > i915_sample                                  756     758      +2
> > intel_guc_submission_disable                 364     365      +1
> > igt_mmap_offset_exhaustion                  1035    1036      +1
> > i915_runtime_pm_status                       257     258      +1
> > i915_rps_boost_info                         1358    1359      +1
> > i915_hangcheck_info                         1229    1230      +1
> > i915_gem_switch_to_kernel_context            682     683      +1
> > i915_gem_suspend                             410     411      +1
> > i915_gem_resume                              254     255      +1
> > i915_gem_park                                190     191      +1
> > i915_engine_info                             279     280      +1
> > intel_rps_mark_interactive                   194     193      -1
> > i915_hangcheck_elapsed                      1526    1525      -1
> > i915_gem_wait_for_idle                       298     297      -1
> > i915_drop_caches_set                         555     554      -1
> > execlists_submission_tasklet                 126     125      -1
> > aliasing_gtt_bind_vma                        235     234      -1
> > i915_gem_retire_work_handler                 144     142      -2
> > igt_evict_contexts.part                      916     910      -6
> > intel_runtime_pm_get_if_in_use               141      23    -118
> > intel_runtime_pm_put                         136       -    -136
> 
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0ebde13620cb..41d253e8c09e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -139,6 +139,8 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> >  
> >  static u32 __i915_gem_park(struct drm_i915_private *i915)
> >  {
> > +     intel_wakeref_t wakeref;
> > +
> >       GEM_TRACE("\n");
> >  
> >       lockdep_assert_held(&i915->drm.struct_mutex);
> > @@ -169,14 +171,15 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
> >       i915_pmu_gt_parked(i915);
> >       i915_vma_parked(i915);
> >  
> > -     i915->gt.awake = false;
> > +     wakeref = fetch_and_zero(&i915->gt.awake);
> > +     GEM_BUG_ON(!wakeref);
> 
> Mmh, I wonder if this should warrant a separate patch.
> 
> >  
> >       if (INTEL_GEN(i915) >= 6)
> >               gen6_rps_idle(i915);
> >  
> >       intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ);
> >  
> > -     intel_runtime_pm_put(i915);
> > +     intel_runtime_pm_put(i915, wakeref);
> >  
> >       return i915->gt.epoch;
> >  }
> > @@ -205,7 +208,8 @@ void i915_gem_unpark(struct drm_i915_private *i915)
> >       if (i915->gt.awake)
> >               return;
> >  
> > -     intel_runtime_pm_get_noresume(i915);
> > +     i915->gt.awake = intel_runtime_pm_get_noresume(i915);
> > +     GEM_BUG_ON(!i915->gt.awake);
> 
> Ditto.

I thought this was a simple transformation, and the tracking is a
fundamental part of this patch. There's a later patch to realise we do
the same thing twice and turn it into one.

> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 4288c0e02f0c..44566dc2f9cc 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1364,14 +1364,14 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
> >  
> >       free_oa_buffer(dev_priv);
> >  
> > +     put_oa_config(dev_priv, stream->oa_config);
> > +
> >       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > -     intel_runtime_pm_put(dev_priv);
> > +     intel_runtime_pm_put(dev_priv, stream->wakeref);
> >  
> >       if (stream->ctx)
> >               oa_put_render_ctx_id(stream);
> >  
> > -     put_oa_config(dev_priv, stream->oa_config);
> > -
> 
> Can we extract this ordering change to a separate patch?

Hmm, doesn't look justified. I hope it was a rebase undoing a later
patch.
 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index ac513fd70315..a1e4e1033289 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -29,6 +29,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/hdmi.h>
> >  #include <linux/sched/clock.h>
> > +#include <linux/stackdepot.h>
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  #include <drm/drm_crtc.h>
> > @@ -2182,10 +2183,16 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *i915)
> >       atomic_dec(&i915->runtime_pm.wakeref_count);
> >  }
> >  
> > -void intel_runtime_pm_get(struct drm_i915_private *i915);
> > -bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915);
> > -void intel_runtime_pm_get_noresume(struct drm_i915_private *i915);
> > -void intel_runtime_pm_put(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get_noresume(struct drm_i915_private *i915);
> 
> __must_check would be an interesting annotation for these. It would help
> catch the blunders with DEBUG_RUNTIME_PM=n by ensuring you assign the
> return value somewhere. It's just that not assigning is valid for the
> put_unchecked cases. *shrug*

Yeah, I dare say once we've broken the back of _unchecked and track
wakerefs everywhere (gvt and atomic modesetting are the hold outs), then
it should be must_check.

> > +void intel_runtime_pm_put_unchecked(struct drm_i915_private *i915);
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > +void intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref);
> > +#else
> > +#define intel_runtime_pm_put(i915, wref) intel_runtime_pm_put_unchecked(i915)
> > +#endif
> 
> Normally I'd probably like to see an actual function here, just to get
> the wref type checked, but I presume this helps the compiler toss away
> the local variable.

Right.

> > @@ -94,8 +94,55 @@ track_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
> >       if (stacks) {
> >               stacks[rpm->debug_count++] = stack;
> >               rpm->debug_owners = stacks;
> > +     } else {
> > +             stack = -1;
> >       }
> >       spin_unlock_irqrestore(&rpm->debug_lock, flags);
> > +
> > +     return stack;
> > +}
> > +
> > +static void cancel_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
> > +                                         depot_stack_handle_t stack)
> > +{
> > +     struct i915_runtime_pm *rpm = &i915->runtime_pm;
> > +     unsigned long flags, n;
> > +     bool found = false;
> > +
> > +     if (unlikely(stack == -1))
> > +             return;
> 
> Unlikely of the should not happen magnitude? WARN_ON?

It's following an allocation error, so not improbable but still expected
at some point. But -1 is improbable for any other reason.
 
> > @@ -379,6 +383,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> >       struct igt_spinner spin;
> >       enum intel_engine_id id;
> >       struct i915_request *rq;
> > +     intel_wakeref_t wakeref;
> >       int ret = 0;
> >  
> >       if (!intel_has_reset_engine(i915))
> > @@ -389,7 +394,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> >               return PTR_ERR(ctx);
> >  
> >       igt_global_reset_lock(i915);
> > -     intel_runtime_pm_get(i915);
> > +     wakeref = intel_runtime_pm_get(i915);
> >  
> >       for_each_engine(engine, i915, id) {
> >               bool ok;
> > @@ -417,6 +422,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> >               rq = igt_spinner_create_request(&spin, ctx, engine, MI_NOOP);
> >               if (IS_ERR(rq)) {
> >                       ret = PTR_ERR(rq);
> > +                     intel_runtime_pm_put(i915, wakeref);
> 
> This looks suspect.
> 
> >                       igt_spinner_fini(&spin);
> >                       goto err;
> >               }
> > @@ -425,6 +431,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> >  
> >               if (!igt_wait_for_spinner(&spin, rq)) {
> >                       pr_err("Spinner failed to start\n");
> > +                     intel_runtime_pm_put(i915, wakeref);
> 
> Ditto.

Ta.
-Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux