[Intel-gfx] [PATCH] drm/i915/perf: don't forget noa wait after oa config

[Chris Wilson]

Quoting Lionel Landwerlin (2019-11-13 18:07:59)
> On 13/11/2019 18:35, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2019-11-13 15:46:39)
> >> I'm observing incoherence metric values, changing from run to run.
> >>
> >> It appears the patches introducing noa wait & reconfiguration from
> >> command stream switched places in the series multiple times during the
> >> review. This lead to the dependency of one onto the order to go
> >> missing...
> > I don't think I dropped it; if I did my apologies. I do feel the
> > egg-on-face for writing a selftest to verify that noa_wait does what you
> > said it did, but completely missing that it went unused :)
> >   
> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >> Fixes: 15d0ace1f876 ("drm/i915/perf: execute OA configuration from command stream")
> >> ---
> >>   drivers/gpu/drm/i915/i915_perf.c | 9 +++++++--
> >>   1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >> index 507236bd41ae..31e47ee23357 100644
> >> --- a/drivers/gpu/drm/i915/i915_perf.c
> >> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> @@ -1870,7 +1870,7 @@ alloc_oa_config_buffer(struct i915_perf_stream *stream,
> >>          config_length += num_lri_dwords(oa_config->mux_regs_len);
> >>          config_length += num_lri_dwords(oa_config->b_counter_regs_len);
> >>          config_length += num_lri_dwords(oa_config->flex_regs_len);
> >> -       config_length++; /* MI_BATCH_BUFFER_END */
> >> +       config_length += 3; /* MI_BATCH_BUFFER_START */
> >>          config_length = ALIGN(sizeof(u32) * config_length, I915_GTT_PAGE_SIZE);
> >>   
> >>          obj = i915_gem_object_create_shmem(stream->perf->i915, config_length);
> >> @@ -1895,7 +1895,12 @@ alloc_oa_config_buffer(struct i915_perf_stream *stream,
> >>                               oa_config->flex_regs,
> >>                               oa_config->flex_regs_len);
> >>   
> >> -       *cs++ = MI_BATCH_BUFFER_END;
> >> +       /* Jump into the active wait. */
> >> +       *cs++ = (INTEL_GEN(stream->perf->i915) < 8 ?
> >> +                MI_BATCH_BUFFER_START :
> >> +                MI_BATCH_BUFFER_START_GEN8);
> >> +       *cs++ = i915_ggtt_offset(stream->noa_wait);
> >> +       *cs++ = 0;
> > Yikes, stream->noa_wait is unused.
> >
> > Hmm, the noa_wait doesn't have any arbitration points internally, so we
> > probably do need to make it non-preemptable as well?
> >
> > With a rq->flags |= I915_REQUEST_NOPREEMPT in emit_oa_config,
> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> >
> > We need to wrap emit_oa_config() in a similar selftest and verify
> > that a read of the oa regs are correct and that the TIMESTAMP indicates
> > the appropriate delay before the read. If you feel bored.
> > -Chris
> 
> 
> As long as we wait long enough, it should be okay.
> 
> Why making it nopreempt?

Aiui, the batch buffer has no arbitration points so the delay may incur
the wrath of the forced preemption. That is another request (of higher
priority) wishing to run, but not being able to.
-Chris