On Tue, 12 Sep 2023 18:46:12 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh,
> On Fri, Sep 08, 2023 at 06:24:16PM -0700, Dixit, Ashutosh wrote: > > On Fri, 08 Sep 2023 18:16:26 -0700, Ashutosh Dixit wrote: > >> > > > >> From: Umesh Nerlige Ramappa <umesh.nerlige.rama...@intel.com> > >> > >> Correct values for OAR counters are still dependent on enabling the > >> GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this > >> bit means OAG unit will write reports to the OAG buffer, so > >> initialize the OAG buffer unconditionally for all use cases. > >> > >> BSpec: 46822 > >> > >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.rama...@intel.com> > >> Signed-off-by: Ashutosh Dixit <ashutosh.di...@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_perf.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_perf.c > >> b/drivers/gpu/drm/i915/i915_perf.c > >> index 1347e4ec9dd5a..30cf37d6e79be 100644 > >> --- a/drivers/gpu/drm/i915/i915_perf.c > >> +++ b/drivers/gpu/drm/i915/i915_perf.c > >> @@ -3032,12 +3032,12 @@ static void gen12_oa_enable(struct > >> i915_perf_stream *stream) > >> u32 val; > >> > >> /* > >> - * If we don't want OA reports from the OA buffer, then we don't even > >> - * need to program the OAG unit. > >> + * BSpec: 46822 > >> + * Correct values for OAR counters are still dependent on enabling the > >> + * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this > >> + * bit means OAG unit will write reports to the OAG buffer, so > >> + * initialize the OAG buffer correctly. > >> */ > >> - if (!(stream->sample_flags & SAMPLE_OA_REPORT)) > >> - return; > >> - > >> gen12_init_oa_buffer(stream); > >> > >> regs = __oa_regs(stream); > > > > Looks like this should be needed, I can R-b it. > > > > However, gen12_test_mi_rpc IGT says: > > > > /* OA unit configuration: > > * DRM_I915_PERF_PROP_SAMPLE_OA is no longer required for Gen12 > > * because the OAR unit increments counters only for the > > * relevant context. No other parameters are needed since we do > > * not rely on the OA buffer anymore to normalize the counter > > * values. > > */ > > That's wrong. When TGL support was added, this was misunderstood and I > removed the OAR-OAG dependency. Ideally we should enforce user to pass > SAMPLE_OA always, but now that will break uabi. SAMPLE_OA has other effects like enabling the timer etc. Also, if we enforce user to always pass it, we can do it in the kernel itself and ignore the uapi parameter. > > For for the OAR case, let's just enable OAG unconditionally so that the OAR > counters tick correctly. While we do that, we should disable all events > that trigger a report into the OA buffer. In addition, I would also > allocate the smallest OA buffer size for this case, so that memory impact > is low. > > Needs a Fixes tag with the commit that enabled OA for TGL. If you think this patch is needed, I think it's better you do this so that you can stay the patch author and I can be the reviewer, otherwise we'll need to go hunt for another reviewer. Thanks. -- Ashutosh > > > > So gen12_test_mi_rpc doesn't set DRM_I915_PERF_PROP_SAMPLE_OA and also > > seems to be passing in CI (don't see it but there seem to be no open > > bugs). Thoughts? > > > > Thanks. > > -- > > Ashutosh
