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

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

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.

>
> 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
>