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

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

On Mon, Nov 18, 2024 at 05:32:56PM -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).
> 

This is something we will need to keep on eye on then because in various
experimental code I've played around enabling exec queues upon creation.
e.g., If we want to allocate a doorbell. I seem to recall Habana wanting
to enable exec queues upon creation too.

Just curious if it was ever explored having exec queue creation
extension which enables OA? It seems like this is something we may need
at some point if our exec queue creation semantics change of course
being careful to not break existing flows.

Matt

> 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.
> 
> v2: Don't export exec_queue_enabled, define new xe_exec_queue_op (M Brost)
>     Directly check OAC_CONTEXT_ENABLE bit from context image (J Cavitt)
> 
> 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_exec_queue_types.h |  2 ++
>  drivers/gpu/drm/xe/xe_guc_submit.c       |  1 +
>  drivers/gpu/drm/xe/xe_oa.c               | 13 +++++++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index 1158b6062a6cd..b88d617c37b33 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -184,6 +184,8 @@ struct xe_exec_queue_ops {
>  	void (*resume)(struct xe_exec_queue *q);
>  	/** @reset_status: check exec queue reset status */
>  	bool (*reset_status)(struct xe_exec_queue *q);
> +	/** @enabled: check if exec queue is in enabled state */
> +	bool (*enabled)(struct xe_exec_queue *q);
>  };
>  
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index f9ecee5364d82..b9b9cdb6f768b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1660,6 +1660,7 @@ static const struct xe_exec_queue_ops guc_exec_queue_ops = {
>  	.suspend_wait = guc_exec_queue_suspend_wait,
>  	.resume = guc_exec_queue_resume,
>  	.reset_status = guc_exec_queue_reset_status,
> +	.enabled = exec_queue_enabled,
>  };
>  
>  static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 8dd55798ab312..4a7440c40978c 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -2066,6 +2066,19 @@ 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 sets the
> +		 * OAC_CONTEXT_ENABLE bit in CTXT_SR_CTL register. Toggling the bit changes
> +		 * the size and layout of the underlying HW context image and can cause hangs.
> +		 */
> +		if (XE_IOCTL_DBG(oa->xe,
> +				 !(xe_lrc_read_ctx_reg(param.exec_q->lrc[0],
> +				       CTX_CONTEXT_CONTROL) & CTX_CTRL_OAC_CONTEXT_ENABLE) &&
> +				 param.exec_q->ops->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
>