Re: [PATCH 1/8] drm/xe/oa: Separate batch submission from waiting for completion

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

On Thu, 08 Aug 2024 13:18:21 -0700, Cavitt, Jonathan wrote:
>

Hi Jonathan,

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Thursday, August 8, 2024 10:42 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; Souza, Jose <jose.souza@intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
> Subject: [PATCH 1/8] drm/xe/oa: Separate batch submission from waiting for completion
> >
> > When we introduce xe_syncs, we don't wait for internal OA programming
> > batches to complete. That is, xe_syncs are signaled asynchronously. In
> > anticipation for this, separate out batch submission from waiting for
> > completion of those batches.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> The added goto err in xe_oa_emit_oa_config had me briefly
> worried due to the fence variable not getting freed, but it
> seems the new organization of the series never sets the
> fence variable in the case of an error.

The patch still had a bug. fence should be initialized to NULL for
xe_bb_free() and dma_fence_put() to work if xe_oa_submit_bb() returns
error. Hopefully fixed in v2 where the patch is reworked along lines
suggested by Matt.

> So, LGTM.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Thanks.
--
Ashutosh


> -Jonathan Cavitt
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 45 ++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 3ef92eb8fbb1e..d842c801fb9f1 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -563,11 +563,10 @@ static __poll_t xe_oa_poll(struct file *file, poll_table *wait)
> >	return ret;
> >  }
> >
> > -static int xe_oa_submit_bb(struct xe_oa_stream *stream, struct xe_bb *bb)
> > +static int xe_oa_submit_bb(struct xe_oa_stream *stream, struct xe_bb *bb,
> > +			   struct dma_fence **fence)
> >  {
> >	struct xe_sched_job *job;
> > -	struct dma_fence *fence;
> > -	long timeout;
> >	int err = 0;
> >
> >	/* Kernel configuration is issued on stream->k_exec_q, not stream->exec_q */
> > @@ -578,15 +577,8 @@ static int xe_oa_submit_bb(struct xe_oa_stream *stream, struct xe_bb *bb)
> >	}
> >
> >	xe_sched_job_arm(job);
> > -	fence = dma_fence_get(&job->drm.s_fence->finished);
> > +	*fence = dma_fence_get(&job->drm.s_fence->finished);
> >	xe_sched_job_push(job);
> > -
> > -	timeout = dma_fence_wait_timeout(fence, false, HZ);
> > -	dma_fence_put(fence);
> > -	if (timeout < 0)
> > -		err = timeout;
> > -	else if (!timeout)
> > -		err = -ETIME;
> >  exit:
> >	return err;
> >  }
> > @@ -652,6 +644,7 @@ static void xe_oa_store_flex(struct xe_oa_stream *stream, struct xe_lrc *lrc,
> >  static int xe_oa_modify_ctx_image(struct xe_oa_stream *stream, struct xe_lrc *lrc,
> >				  const struct flex *flex, u32 count)
> >  {
> > +	struct dma_fence *fence;
> >	struct xe_bb *bb;
> >	int err;
> >
> > @@ -663,14 +656,16 @@ static int xe_oa_modify_ctx_image(struct xe_oa_stream *stream, struct xe_lrc *lr
> >
> >	xe_oa_store_flex(stream, lrc, bb, flex, count);
> >
> > -	err = xe_oa_submit_bb(stream, bb);
> > -	xe_bb_free(bb, NULL);
> > +	err = xe_oa_submit_bb(stream, bb, &fence);
> > +	xe_bb_free(bb, fence);
> > +	dma_fence_put(fence);
> >  exit:
> >	return err;
> >  }
> >
> >  static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *reg_lri)
> >  {
> > +	struct dma_fence *fence;
> >	struct xe_bb *bb;
> >	int err;
> >
> > @@ -682,8 +677,9 @@ static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *re
> >
> >	write_cs_mi_lri(bb, reg_lri, 1);
> >
> > -	err = xe_oa_submit_bb(stream, bb);
> > -	xe_bb_free(bb, NULL);
> > +	err = xe_oa_submit_bb(stream, bb, &fence);
> > +	xe_bb_free(bb, fence);
> > +	dma_fence_put(fence);
> >  exit:
> >	return err;
> >  }
> > @@ -913,15 +909,30 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
> >  {
> >  #define NOA_PROGRAM_ADDITIONAL_DELAY_US 500
> >	struct xe_oa_config_bo *oa_bo;
> > -	int err, us = NOA_PROGRAM_ADDITIONAL_DELAY_US;
> > +	int err = 0, us = NOA_PROGRAM_ADDITIONAL_DELAY_US;
> > +	struct dma_fence *fence;
> > +	long timeout;
> >
> > +	/* Emit OA configuration batch */
> >	oa_bo = xe_oa_alloc_config_buffer(stream, config);
> >	if (IS_ERR(oa_bo)) {
> >		err = PTR_ERR(oa_bo);
> >		goto exit;
> >	}
> >
> > -	err = xe_oa_submit_bb(stream, oa_bo->bb);
> > +	err = xe_oa_submit_bb(stream, oa_bo->bb, &fence);
> > +	if (err)
> > +		goto exit;
> > +
> > +	/* Wait till all previous batches have executed */
> > +	timeout = dma_fence_wait_timeout(fence, false, 5 * HZ);
> > +	dma_fence_put(fence);
> > +	if (timeout < 0)
> > +		err = timeout;
> > +	else if (!timeout)
> > +		err = -ETIME;
> > +	if (err)
> > +		drm_dbg(&stream->oa->xe->drm, "dma_fence_wait_timeout err %d\n", err);
> >
> >	/* Additional empirical delay needed for NOA programming after registers are written */
> >	usleep_range(us, 2 * us);
> > --
> > 2.41.0
> >
> >