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

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

On Thu, Aug 08, 2024 at 10:41:33AM -0700, Ashutosh Dixit wrote:
> 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>
> ---
>  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 {

See my comment here [1], quite sure this is not needed.

Matt

[1] https://patchwork.freedesktop.org/patch/607602/?series=137058&rev=1#comment_1104294

> +	/* @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
>