Re: [PATCH 5/8] drm/xe/oa: Signal output fences

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

On Thu, Aug 08, 2024 at 10:41:36AM -0700, Ashutosh Dixit wrote:
> Complete 'struct xe_oa_fence' to include the dma_fence used to signal
> output fences in the xe_sync array. The fences are signalled
> asynchronously. When there are no output fences to signal, the OA
> configuration wait is synchronously re-introduced into the ioctl.
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_oa.c | 46 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 416e031ac454b..bc421cd0af6ba 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -96,6 +96,10 @@ struct xe_oa_config_bo {
>  };
>  
>  struct xe_oa_fence {

I don't think you need this at all.

The fence from the job can directly be installed via
xe_sync_entry_signal between xe_sched_job_arm and
xe_sched_job_push.

Then before xe_sync_entry_cleanup check for !num_signal and directly
wait on the job's fence.

Matt 

> +	/* @base: dma fence base */
> +	struct dma_fence base;
> +	/* @lock: lock for the fence */
> +	spinlock_t lock;
>  	/* @xe: pointer to xe device */
>  	struct xe_device *xe;
>  	/* @work: work to signal that OA configuration is applied */
> @@ -953,9 +957,26 @@ static void xe_oa_fence_work_fn(struct work_struct *w)
>  	/* Additional empirical delay needed for NOA programming after registers are written */
>  	usleep_range(us, 2 * us);
>  
> -	kfree(ofence);
> +	/* Now signal fence to indicate new OA configuration is active */
> +	dma_fence_signal(&ofence->base);
> +	dma_fence_put(&ofence->base);
>  }
>  
> +static const char *xe_oa_get_driver_name(struct dma_fence *fence)
> +{
> +	return "xe_oa";
> +}
> +
> +static const char *xe_oa_get_timeline_name(struct dma_fence *fence)
> +{
> +	return "unbound";
> +}
> +
> +static const struct dma_fence_ops xe_oa_fence_ops = {
> +	.get_driver_name = xe_oa_get_driver_name,
> +	.get_timeline_name = xe_oa_get_timeline_name,
> +};
> +
>  static struct xe_oa_fence *xe_oa_fence_init(struct xe_device *xe, struct dma_fence *config_fence)
>  {
>  	struct xe_oa_fence *ofence;
> @@ -967,6 +988,8 @@ static struct xe_oa_fence *xe_oa_fence_init(struct xe_device *xe, struct dma_fen
>  	ofence->xe = xe;
>  	INIT_WORK(&ofence->work, xe_oa_fence_work_fn);
>  	ofence->config_fence = config_fence;
> +	spin_lock_init(&ofence->lock);
> +	dma_fence_init(&ofence->base, &xe_oa_fence_ops, &ofence->lock, 0, 0);
>  
>  	return ofence;
>  }
> @@ -975,8 +998,8 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
>  {
>  	struct xe_oa_config_bo *oa_bo;
>  	struct xe_oa_fence *ofence;
> +	int i, err, num_signal = 0;
>  	struct dma_fence *fence;
> -	int err;
>  
>  	/* Emit OA configuration batch */
>  	oa_bo = xe_oa_alloc_config_buffer(stream, config);
> @@ -989,13 +1012,30 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
>  	if (err)
>  		goto exit;
>  
> +	/* Initialize and set fence to signal */
>  	ofence = xe_oa_fence_init(stream->oa->xe, fence);
>  	if (IS_ERR(ofence)) {
>  		err = PTR_ERR(ofence);
>  		goto put_fence;
>  	}
>  
> -	xe_oa_fence_work_fn(&ofence->work);
> +	for (i = 0; i < stream->num_syncs; i++)
> +		xe_sync_entry_signal(&stream->syncs[i], &ofence->base);
> +
> +	/* Schedule work to signal the fence */
> +	queue_work(system_unbound_wq, &ofence->work);
> +
> +	/* If nothing needs to be signaled we wait synchronously */
> +	for (i = 0; i < stream->num_syncs; i++)
> +		if (stream->syncs[i].flags & DRM_XE_SYNC_FLAG_SIGNAL)
> +			num_signal++;
> +	if (!num_signal)
> +		flush_work(&ofence->work);
> +
> +	/* Done with syncs */
> +	for (i = 0; i < stream->num_syncs; i++)
> +		xe_sync_entry_cleanup(&stream->syncs[i]);
> +	kfree(stream->syncs);
>  
>  	return 0;
>  put_fence:
> -- 
> 2.41.0
>