Re: [PATCH 1/2] drm/xe/oa: Disallow OA from being enabled on active exec_queue's

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

On Fri, Nov 08, 2024 at 03:06:00PM -0800, Ashutosh Dixit wrote:
> Enabling OA on an exec_queue toggles the OAC_CONTEXT_ENABLE bit in
> CTXT_SR_CTL register. Toggling this bit changes the size and layout of the
> underlying HW context image. Therefore, enabling OA on an already active
> exec_queue (as currently implemented in xe) is an invalid operation and can
> cause hangs. Therefore, disallow OA from being enabled on active
> exec_queue's (here by active we mean a context on which submissions have
> previously happened).
> 
> Transition from 1 -> 0 for this bit was disallowed in
> '0c8650b09a36 ("drm/xe/oa: Don't reset OAC_CONTEXT_ENABLE on OA stream
> close")'. Here we disallow the 0 -> 1 transition on active contexts.
> 
> Bspec: 60314
> Fixes: 2f4a730fcd2d ("drm/xe/oa: Add OAR support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c |  2 +-
>  drivers/gpu/drm/xe/xe_guc_submit.h |  1 +
>  drivers/gpu/drm/xe/xe_oa.c         | 11 +++++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9e0f86f3778b2..5c285d966fd5c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -83,7 +83,7 @@ static void clear_exec_queue_registered(struct xe_exec_queue *q)
>  	atomic_and(~EXEC_QUEUE_STATE_REGISTERED, &q->guc->state);
>  }
>  
> -static bool exec_queue_enabled(struct xe_exec_queue *q)
> +bool exec_queue_enabled(struct xe_exec_queue *q)

Don't export this. Use a xe_exec_queue_ops vfunc. Current vfunc
'reset_status' would kinda similar to this. A vfunc protects use against
baking in submission backend specifics into other layers. We only really
have 1 ATM that works (execlists backend is still here for some reason)
but this is the best practice.

Matt

>  {
>  	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_ENABLED;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.h b/drivers/gpu/drm/xe/xe_guc_submit.h
> index 9b71a986c6ca6..a3bde061dd53c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.h
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.h
> @@ -12,6 +12,7 @@ struct drm_printer;
>  struct xe_exec_queue;
>  struct xe_guc;
>  
> +bool exec_queue_enabled(struct xe_exec_queue *q);
>  int xe_guc_submit_init(struct xe_guc *guc, unsigned int num_ids);
>  
>  int xe_guc_submit_reset_prepare(struct xe_guc *guc);
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index fd2ffe8df1561..b0692b8ca0a3d 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -28,6 +28,7 @@
>  #include "xe_gt_mcr.h"
>  #include "xe_gt_printk.h"
>  #include "xe_guc_pc.h"
> +#include "xe_guc_submit.h"
>  #include "xe_lrc.h"
>  #include "xe_macros.h"
>  #include "xe_mmio.h"
> @@ -2064,6 +2065,16 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
>  		if (XE_IOCTL_DBG(oa->xe, !param.exec_q))
>  			return -ENOENT;
>  
> +		/*
> +		 * Disallow OA from being enabled on active exec_queue's. Enabling OA toggles
> +		 * the OAC_CONTEXT_ENABLE bit in CTXT_SR_CTL register, which changes the size
> +		 * and layout of the underlying HW context image and can cause hangs.
> +		 */
> +		if (XE_IOCTL_DBG(oa->xe, exec_queue_enabled(param.exec_q))) {
> +			ret = -EADDRINUSE;
> +			goto err_exec_q;
> +		}
> +
>  		if (param.exec_q->width > 1)
>  			drm_dbg(&oa->xe->drm, "exec_q->width > 1, programming only exec_q->lrc[0]\n");
>  	}
> -- 
> 2.41.0
>