Re: [PATCH v5 1/2] xe/oa: Fix query mode of operation for OAR/OAC

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

On Fri, 20 Dec 2024 10:08:41 -0800, Cavitt, Jonathan wrote:
>
>
> I don't have any major blocking revision notes for this.  Feel free to
> take or leave any of the questions/suggestions I left below.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Thanks.

>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c              | 136 ++++++++----------------
> >  drivers/gpu/drm/xe/xe_ring_ops.c        |   5 +-
> >  drivers/gpu/drm/xe/xe_sched_job_types.h |   2 +
> >  3 files changed, 51 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index ae94490b0eac..9add60097ab5 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -74,12 +74,6 @@ struct xe_oa_config {
> >	struct rcu_head rcu;
> >  };
> >
> > -struct flex {
> > -	struct xe_reg reg;
> > -	u32 offset;
> > -	u32 value;
> > -};
> > -
> >  struct xe_oa_open_param {
> >	struct xe_file *xef;
> >	u32 oa_unit_id;
> > @@ -605,19 +599,38 @@ static __poll_t xe_oa_poll(struct file *file, poll_table *wait)
> >	return ret;
> >  }
> >
> > +static void xe_oa_lock_vma(struct xe_exec_queue *q)
> > +{
> > +	if (q->vm) {
> > +		down_read(&q->vm->lock);
> > +		xe_vm_lock(q->vm, false);
> > +	}
> > +}
> > +
> > +static void xe_oa_unlock_vma(struct xe_exec_queue *q)
> > +{
> > +	if (q->vm) {
> > +		xe_vm_unlock(q->vm);
> > +		up_read(&q->vm->lock);
> > +	}
> > +}
> > +
> >  static struct dma_fence *xe_oa_submit_bb(struct xe_oa_stream *stream, enum xe_oa_submit_deps deps,
> >					 struct xe_bb *bb)
> >  {
> > +	struct xe_exec_queue *q = stream->exec_q ?: stream->k_exec_q;
> >	struct xe_sched_job *job;
> >	struct dma_fence *fence;
> >	int err = 0;
> >
> > -	/* Kernel configuration is issued on stream->k_exec_q, not stream->exec_q */
> > -	job = xe_bb_create_job(stream->k_exec_q, bb);
> > +	xe_oa_lock_vma(q);
> > +
> > +	job = xe_bb_create_job(q, bb);
> >	if (IS_ERR(job)) {
> >		err = PTR_ERR(job);
> >		goto exit;
> >	}
> > +	job->ggtt = true;
> >
> >	if (deps == XE_OA_SUBMIT_ADD_DEPS) {
> >		for (int i = 0; i < stream->num_syncs && !err; i++)
> > @@ -632,10 +645,13 @@ static struct dma_fence *xe_oa_submit_bb(struct xe_oa_stream *stream, enum xe_oa
> >	fence = dma_fence_get(&job->drm.s_fence->finished);
> >	xe_sched_job_push(job);
> >
> > +	xe_oa_unlock_vma(q);
> > +
> >	return fence;
> >  err_put_job:
> >	xe_sched_job_put(job);
> >  exit:
> > +	xe_oa_unlock_vma(q);
> >	return ERR_PTR(err);
>
> Non-blocking note:
> Hmm... I'm not a big fan of needing to call xe_oa_unlock_vma for the
> two separate cases, though unless we wanted to completely
> restructure the ending sequence here, I think this is the best option.
>
> Though, I guess this is what the ending sequence restructuring would
> look like if we took that route:
> """
>	fence = dma_fence_get(&job->drm.s_fence->finished);
>	xe_sched_job_push(job);
>
> err_put_job:
>	if (err)
>		xe_sched_job_put(job);
> exit:
>	xe_oa_unlock_vma(q);
>	return err ? ERR_PTR(err) : fence;
> }
> """
> So it seems restructuring the exit sequence would be less of a bad option than
> I had initially thought, but it's not a necessary change.

About this, I think we are going to leave as is for now. But if interested
look at include/linux/cleanup.h for some other ideas.

Thanks.
--
Ashutosh