Re: [PATCH v4 08/13] drm/xe/hw_engine_group: Ensure safe transition between execution modes

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

On Thu, Aug 01, 2024 at 02:56:49PM +0200, Francois Dugast wrote:
> Provide a way to safely transition execution modes of the hw engine group
> ahead of the actual execution. When necessary, either wait for running
> jobs to complete or preempt them, thus ensuring mutual exclusion between
> execution modes.
> 
> Unlike a mutex, the rw_semaphore used in this context allows multiple
> submissions in the same mode.
> 
> v2: Use lockdep_assert_held_write, add annotations (Matt Brost)
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine_group.c | 71 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine_group.h |  5 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_group.c b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> index de057c98d5a5..a5754e6de7c9 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_group.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> @@ -208,3 +208,74 @@ static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group
>  
>  	return 0;
>  }
> +
> +static int switch_mode(struct xe_hw_engine_group *group,
> +		       enum xe_hw_engine_group_execution_mode new_mode)
> +{
> +	int err = 0;
> +
> +	lockdep_assert_held_write(&group->mode_sem);
> +
> +	if (group->cur_mode == EXEC_MODE_LR && new_mode == EXEC_MODE_DMA_FENCE)

This seems redunant, how about:

switch (group->cur_mode) {
case EXEC_MODE_LR:
	xe_hw_engine_group_suspend_faulting_lr_jobs(group);
	break;
case EXEC_MODE_DMA_FENCE:
	err = xe_hw_engine_group_wait_for_dma_fence_jobs(group);
	break;
}

Matt

> +		xe_hw_engine_group_suspend_faulting_lr_jobs(group);
> +	else if (group->cur_mode == EXEC_MODE_DMA_FENCE && new_mode == EXEC_MODE_LR)
> +		err = xe_hw_engine_group_wait_for_dma_fence_jobs(group);
> +	else
> +		err = -EINVAL;
> +
> +	if (err)
> +		return err;
> +
> +	group->cur_mode = new_mode;
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_get_mode() - Get the group to execute in the new mode
> + * @group: The hw engine group
> + * @mode: The new execution mode
> + * @previous_mode: Pointer to the previous mode provided for use by caller
> + *
> + * Return: 0 if successful, -EINTR if locking failed.
> + */
> +int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> +				enum xe_hw_engine_group_execution_mode new_mode,
> +				enum xe_hw_engine_group_execution_mode *previous_mode)
> +__acquires(&group->mode_sem)
> +{
> +	int err = down_read_interruptible(&group->mode_sem);
> +
> +	if (err)
> +		return err;
> +
> +	*previous_mode = group->cur_mode;
> +
> +	if (new_mode != group->cur_mode) {
> +		up_read(&group->mode_sem);
> +		err = down_write_killable(&group->mode_sem);
> +		if (err)
> +			return err;
> +
> +		if (new_mode != group->cur_mode) {
> +			err = switch_mode(group, new_mode);
> +			if (err) {
> +				up_write(&group->mode_sem);
> +				return err;
> +			}
> +		}
> +		downgrade_write(&group->mode_sem);
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * xe_hw_engine_group_put() - Put the group
> + * @group: The hw engine group
> + */
> +void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
> +__releases(&group->mode_sem)
> +{
> +	up_read(&group->mode_sem);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_group.h b/drivers/gpu/drm/xe/xe_hw_engine_group.h
> index 857a83787504..e0deb7c7bb5b 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_group.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_group.h
> @@ -17,4 +17,9 @@ int xe_hw_engine_setup_groups(struct xe_gt *gt);
>  int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
>  void xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
>  
> +int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> +				enum xe_hw_engine_group_execution_mode new_mode,
> +				enum xe_hw_engine_group_execution_mode *previous_mode);
> +void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
> +
>  #endif
> -- 
> 2.43.0
>