Re: [PATCH] drm/xe/oa: Insert wmb/sfence before enabling OA

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

On Tue, 10 Sep 2024 09:14:11 -0700, Matthew Brost wrote:
>

Hi Matt,

> On Mon, Sep 09, 2024 at 07:59:07PM -0700, Ashutosh Dixit wrote:
> > We are occasionally seeing that OA Buffer register is not programmed (has
> > value 0) when OA is enabled. This means OA has been enabled before it has
> > been fully configured. Or, the register write enabling OA has overtaken
> > previous OA configuration register writes.
> >
> > Therefore, insert a wmb/sfence to preserve OA register write ordering
> > before enabling OA.
> >
> > v2: s/wmb()/xe_device_wmb()/
> >
> > Fixes: e936f885f1e9 ("drm/xe/oa/uapi: Expose OA stream fd")
> > Reported-by: Guy Zadicario <guy.zadicario@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 63286ed8457fa..4fb7aae37a94f 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -440,6 +440,9 @@ static void xe_oa_enable(struct xe_oa_stream *stream)
> >	val = __format_to_oactrl(format, regs->oa_ctrl_counter_select_mask) |
> >		__oa_ccs_select(stream) | OAG_OACONTROL_OA_COUNTER_ENABLE;
> >
> > +	/* Flush previous writes to HW before enabling OA */
> > +	xe_device_wmb(stream->oa->xe);
> > +
>
> So is preventing the xe_mmio_write32 passing the memset in
> xe_oa_init_oa_buffer. Indeed a xe_device_wmb is needed.

Actually, what seems to be happening is that the final register write below
which enables OA is passing previous register writes in
xe_oa_init_oa_buffer() which are configuring OA. So in some environments OA
gets enabled before it is fully configured (specifically the OA buffer ggtt
address register set in xe_oa_init_oa_buffer() is occasionally being seen
as 0). xe_device_wmb() is supposed to order these writes.

(There is a spinlock in xe_oa_init_oa_buffer(), but afaik spinlock would
only ensure a smp_wmb() but not a full wmb(), so a wmb() is still needed).

I am still trying to verify if this patch actually fixes the issue and
won't merge this till this is verified. The uncertainty I have is if I
should also do a mmio read to also flush PCI write transactions in addition
to the wmb().

>
> I will say it a bit goofy that stream->oa_buffer.vaddr is accessed
> directly in the OA code rather than using the xe_map layer which made it
> a little harder to reach the above conclusion. You might want to fixup
> the OA to use the xe_map layer rather than directly accessing pointers
> if that is possible.

Will look into this too.

>
> Anyways, this LGTM:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
> >	xe_mmio_write32(stream->gt, regs->oa_ctrl, val);
> >  }
> >
> > --
> > 2.41.0
> >

Thanks.
--
Ashutosh