Re: [PATCH 2/8] drm/xe/oa: Introduce 'struct xe_oa_fence'

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

On Thu, 08 Aug 2024 13:58:52 -0700, Cavitt, Jonathan wrote:
>
> -----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 2/8] drm/xe/oa: Introduce 'struct xe_oa_fence'
> >
> > Here we introduce 'struct xe_oa_fence' which will contain the fence used
> > for signalling xe_syncs (in a later patch). The struct also contains the
> > work struct used for signalling the fences. Otherwise, this patch is a
> > simple refactor of the previous patch. In this patch the work function is
> > executed synchronously.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> If I understand correctly, it seems this is moving the fence
> wait and put section to part of a work fini function that
> executes when the xe_oa_fence is closed?

The work function is scheduled to run, so that the wait can be executed
asynchronously after which the fence will be signalled (in Patch 5/8).

Though there are few changes here in v2, but the basic concept is the
same. See Patch 4 "drm/xe/oa: Signal output fences" in v2.

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

Thanks.
--
Ashutosh


> -Jonathan Cavitt
>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 73 ++++++++++++++++++++++++++++++--------
> >  1 file changed, 59 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index d842c801fb9f1..f97d64ffb460f 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -90,6 +90,15 @@ struct xe_oa_config_bo {
> >	struct xe_bb *bb;
> >  };
> >
> > +struct xe_oa_fence {
> > +	/* @xe: pointer to xe device */
> > +	struct xe_device *xe;
> > +	/* @work: work to signal that OA configuration is applied */
> > +	struct work_struct work;
> > +	/* @config_fence: dma fence for OA config to be applied */
> > +	struct dma_fence *config_fence;
> > +};
> > +
> >  #define DRM_FMT(x) DRM_XE_OA_FMT_TYPE_##x
> >
> >  static const struct xe_oa_format oa_formats[] = {
> > @@ -905,14 +914,51 @@ xe_oa_alloc_config_buffer(struct xe_oa_stream *stream, struct xe_oa_config *oa_c
> >	return oa_bo;
> >  }
> >
> > -static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config *config)
> > +static void xe_oa_fence_work_fn(struct work_struct *w)
> >  {
> >  #define NOA_PROGRAM_ADDITIONAL_DELAY_US 500
> > -	struct xe_oa_config_bo *oa_bo;
> > +	struct xe_oa_fence *ofence = container_of(w, typeof(*ofence), work);
> >	int err = 0, us = NOA_PROGRAM_ADDITIONAL_DELAY_US;
> > -	struct dma_fence *fence;
> >	long timeout;
> >
> > +	/* Wait till all previous batches have executed */
> > +	timeout = dma_fence_wait_timeout(ofence->config_fence, false, 5 * HZ);
> > +	dma_fence_put(ofence->config_fence);
> > +	if (timeout < 0)
> > +		err = timeout;
> > +	else if (!timeout)
> > +		err = -ETIME;
> > +	if (err)
> > +		drm_dbg(&ofence->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);
> > +
> > +	kfree(ofence);
> > +}
> > +
> > +static struct xe_oa_fence *xe_oa_fence_init(struct xe_device *xe, struct dma_fence *config_fence)
> > +{
> > +	struct xe_oa_fence *ofence;
> > +
> > +	ofence = kzalloc(sizeof(*ofence), GFP_KERNEL);
> > +	if (!ofence)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ofence->xe = xe;
> > +	INIT_WORK(&ofence->work, xe_oa_fence_work_fn);
> > +	ofence->config_fence = config_fence;
> > +
> > +	return ofence;
> > +}
> > +
> > +static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config *config)
> > +{
> > +	struct xe_oa_config_bo *oa_bo;
> > +	struct xe_oa_fence *ofence;
> > +	struct dma_fence *fence;
> > +	int err;
> > +
> >	/* Emit OA configuration batch */
> >	oa_bo = xe_oa_alloc_config_buffer(stream, config);
> >	if (IS_ERR(oa_bo)) {
> > @@ -924,18 +970,17 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
> >	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);
> > +	ofence = xe_oa_fence_init(stream->oa->xe, fence);
> > +	if (IS_ERR(ofence)) {
> > +		err = PTR_ERR(ofence);
> > +		goto put_fence;
> > +	}
> >
> > -	/* Additional empirical delay needed for NOA programming after registers are written */
> > -	usleep_range(us, 2 * us);
> > +	xe_oa_fence_work_fn(&ofence->work);
> > +
> > +	return 0;
> > +put_fence:
> > +	dma_fence_put(fence);
> >  exit:
> >	return err;
> >  }
> > --
> > 2.41.0
> >
> >