Re: [RFC v1 5/9] drm/xe/hw_engine_group: Add helper to wait for dma fence jobs

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

On Wed, Jul 17, 2024 at 03:07:26PM +0200, Francois Dugast wrote:
> This is a required feature for long running jobs not to be submitted
> while dma fence jobs are running on the hw engine group.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 850f7b15b154..dc75dfe6187a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1251,3 +1251,30 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
>  
>  	return 0;
>  }
> +
> +/**
> + * xe_hw_engine_group_wait_for_dma_fence_jobs() - Wait for dma fence jobs to complete
> + * @group: The hw engine group
> + *
> + * Return: 0 on success,
> + *	   -ETIME if waiting for one job failed
> + */
> +static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group *group)
> +{
> +	long timeout;
> +	struct xe_exec_queue *q;
> +
> +	lockdep_assert_held(&group->mode_sem);
> +
> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (xe_vm_in_lr_mode(q->vm))
> +			continue;
> +
> +		timeout = dma_fence_wait(q->last_fence, false);

Hmm, I don't think this is safe to just reference q->last_fence because
it can be NULL or for locking reasons.

See xe_exec_queue_last_fence_lockdep_assert for the current locking
requirments.

We are going to have to rethinking
xe_exec_queue_last_fence_lockdep_assert. I think we can safely reference
q->last_fence if this function is called in group->mode_sem in write
mode. Let me think on this and maybe will have more thoughts once I get
through the entire series.

wrt to dma_fence_wait not being interruptable, if this is called from a
user IOCTL can can't really do that. Same goes for suspend_wait in the
previous patch, we'd have to make that interruptable as well. Again let
me get through the entire series and maybe will have more thoughts here.

Matt

> +
> +		if (timeout < 0)
> +			return -ETIME;
> +	}
> +
> +	return 0;
> +}
> -- 
> 2.43.0
>