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

[Matthew Brost <matthew.brost@intel.com>]

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.

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.

Anyways, this LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  	xe_mmio_write32(stream->gt, regs->oa_ctrl, val);
>  }
>  
> -- 
> 2.41.0
>