[Intel-gfx] [PATCH 4/7] drm/i915/perf: lock powergating configuration to default when active

[Chris Wilson]

Quoting Lionel Landwerlin (2018-09-06 10:57:47)
> On 05/09/2018 15:22, Tvrtko Ursulin wrote:
> > From: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >
> > If some of the contexts submitting workloads to the GPU have been
> > configured to shutdown slices/subslices, we might loose the NOA
> > configurations written in the NOA muxes.
> >
> > One possible solution to this problem is to reprogram the NOA muxes
> > when we switch to a new context. We initially tried this in the
> > workaround batchbuffer but some concerns where raised about the cost
> > of reprogramming at every context switch. This solution is also not
> > without consequences from the userspace point of view. Reprogramming
> > of the muxes can only happen once the powergating configuration has
> > changed (which happens after context switch). This means for a window
> > of time during the recording, counters recorded by the OA unit might
> > be invalid. This requires userspace dealing with OA reports to discard
> > the invalid values.
> >
> > Minimizing the reprogramming could be implemented by tracking of the
> > last programmed configuration somewhere in GGTT and use MI_PREDICATE
> > to discard some of the programming commands, but the command streamer
> > would still have to parse all the MI_LRI instructions in the
> > workaround batchbuffer.
> >
> > Another solution, which this change implements, is to simply disregard
> > the user requested configuration for the period of time when i915/perf
> > is active. There is no known issue with this apart from a performance
> > penality for some media workloads that benefit from running on a
> > partially powergated GPU. We already prevent RC6 from affecting the
> > programming so it doesn't sound completely unreasonable to hold on
> > powergating for the same reason.
> >
> > v2: Leave RPCS programming in intel_lrc.c (Lionel)
> >
> > v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
> >      More to_intel_context() (Tvrtko)
> >      s/dev_priv/i915/ (Tvrtko)
> >
> > Tvrtko Ursulin:
> >
> > v4:
> >   * Rebase for make_rpcs changes.
> >
> > v5:
> >   * Apply OA restriction from make_rpcs directly.
> >
> > v6:
> >   * Rebase for context image setup changes.
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c |  5 +++++
> >   drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
> >   drivers/gpu/drm/i915/intel_lrc.h |  3 +++
> >   3 files changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index ccb20230df2c..dd65b72bddd4 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1677,6 +1677,11 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
> >   
> >               CTX_REG(reg_state, state_offset, flex_regs[i], value);
> >       }
> > +
> > +     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
> > +             gen8_make_rpcs(dev_priv,
> > +                            &to_intel_context(ctx,
> > +                                              dev_priv->engine[RCS])->sseu));
> 
> 
> I think there is one issue I missed on the previous iterations of this 
> patch.
> 
> This gen8_update_reg_state_unlocked() is called when the GPU is parked 
> on the kernel context.
> 
> It's supposed to update all contexts, but I think we might not be able 
> to update the kernel context image while the GPU is using it.

The kernel context is only ever taken in extremis (you are either
parking or stalling userspace) so I don't care.
 
> Context save might happen after we edited the image and that would 
> override the values we just put in there.
> 
> 
> The OA config is emitted through context image edition in this function 
> but also through the ring buffer in 
> gen8_switch_to_updated_kernel_context() for the kernel context.
> 
> Since we can't have a context modify its own RCPS value, we'll have to 
> resort to yet another context to do that for the kernel context.
> 
> 
> I remember having a patch that created yet another kernel context (let's 
> call it rpcs edition context), which is used to reconfigure rpcs for 
> every context but itself and then have the kernel context reconfigure 
> this rpcs edition context.
> 
> Or alternatively not do anything to it, because it's only going to run 
> to edit other contexts at a time when we don't care about power 
> configuration stability.

Exactly.
-Chris