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

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

On Fri, 08 Nov 2024 15:14:46 -0800, Matthew Brost wrote:
>
> 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.

Done in v2, thanks for the suggestion, Matt. I've only added the vfunc to
GuC though, since OA is disabled for execlists.