Re: [PATCH v5 2/2] tests/intel/xe_oa: Wa_14026633728

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 05 May 2026 21:58:02 -0700, Dixit, Ashutosh wrote:
>
> On Tue, 05 May 2026 19:55:14 -0700, Shekhar Chauhan wrote:
> >
> >
> > On 5/6/2026 8:14, Shekhar Chauhan wrote:
> > > For MERTOA in CRI, oa buffer can be in device memory. Because of slower
> > > device mem access, OA exponent values lower than 8 can result in buffer
> > > overflows. Bump the OA exponent value.
> > >
> > > Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> > > ---
> > >   tests/intel/xe_oa.c | 12 ++++++++++--
> > >   1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> > > index 988c46df6..67db20168 100644
> > > --- a/tests/intel/xe_oa.c
> > > +++ b/tests/intel/xe_oa.c
> > > @@ -24,6 +24,7 @@
> > >   #include "igt_device.h"
> > >   #include "igt_syncobj.h"
> > >   #include "igt_sysfs.h"
> > > +#include "intel_wa.h"
> > >   #include "xe/xe_ioctl.h"
> > >   #include "xe/xe_query.h"
> > >   #include "xe/xe_oa.h"
> > > @@ -2643,8 +2644,15 @@ test_buffer_fill(const struct drm_xe_oa_unit *oau)
> > >   static void
> > >   test_non_zero_reason(const struct drm_xe_oa_unit *oau, size_t oa_buffer_size)
> > >   {
> > > -	/* ~20 micro second period */
> > > -	int oa_exponent = max_oa_exponent_for_period_lte(20000);
> > > +	/*
> > > +	 * Wa_14026633728: For MERTOA in CRI, oa buffer can be in device memory.
> > > +	 * Because of slower device mem access, OA exponent values lower than 8 can
> > > +	 * result in buffer overflows.
> > > +	 * By default, use ~20 micro second period.
> > > +	 */
> > > +	int oa_exponent = max(max_oa_exponent_for_period_lte(20000),
> > > +			      (oau->oa_unit_type == DRM_XE_OA_UNIT_TYPE_MERT &&
> > > +			       igt_has_intel_wa(drm_fd, "14026633728")) ? 8 : 0);
> >
> > I agree that the v2 was more readable. But, the issue in using that again
> > is the C90 rule violation. If at all I declare here and then later modify
> > it, properties[] uses oa_exponent, and I believe it's value should be
> > finalised before declaration. If I place this max condition before that,
> > again a C90 rule violation. The max() condition in the initializer avoids
> > this which is why I didn't make that change. If you have any other ideas,
> > I'm open to suggestions.
>
> Add the if statement after the declarations and modify the property array
> too (there are several examples of this) with the new exponent value.
>
> Note max etc. is not needed, as it was in v2.

Sorry, max is needed, as it was in v2.

>
> >
> > As for the dropping > 0, I still have mixed feelings about that
> > change. igt_has_intel_wa has 3 return values: 0 if no WA is present, 1 if
> > the WA is present and -1 on error (maybe couldn't load from debugfs or some
> > other issue). When I explicitly write it as > 0, it signifies that the
> > workaround was definitely found and it exists. Of course, the return value
> > would always be 1 when the workaround is found, so, technically, the code
> > is still right (and cleaner, I agree).
>
> OK, you are right.
>
> But the function is *very* badly written. A function returning int should
> generally return 0 on success and non-zero on failure. There is no point
> distinguishing between error and no_wa cases. Moreover, error case should
> just be an assert, rather than a return value.
>
> If you want to clean all this up, add a 3rd patch, since you already have a
> R-b from Gustavo on Patch 1. So retain Patch 1 as is for now.
>
> There are no users/callers of the function except for oa, so it's a good
> time to change it now before new callers appear.
>
> >
> > Let me know what you (Ashutosh) think about the oa_exponent declaration vs
> > initialization vs usage thing (in regards to C90) and I can send another
> > patch if a change is required.
> >
> > -shekhar
> >
> > >	struct intel_xe_perf_metric_set *test_set = oa_unit_metric_set(oau);
> > >	uint64_t fmt = test_set->perf_oa_format;
> > >	size_t report_size = get_oa_format(fmt).size;
> >
> > --
> > Shekhar Chauhan
> > Linux Graphics Software Engineer
> > Intel Corporation
> >