Re: [RFC v1 6/9] drm/xe/hw_engine_group: Ensure safe transition between execution modes

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

On Wed, Jul 17, 2024 at 03:07:27PM +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 modes EXEC_MODE_LR and EXEC_MODE_DMA_FENCE.
> 
> Unlike a mutex, the rw_semaphore used in this context allows multiple
> submissions in the same mode.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 67 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h |  3 ++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index dc75dfe6187a..4f539711357a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1278,3 +1278,70 @@ 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(&group->mode_sem);

lockdep_assert_held_write

> +
> +	if (group->cur_mode == new_mode)
> +		return 0;

This is redunant as the caller checks this.

> +	else if (group->cur_mode == EXEC_MODE_LR && new_mode == EXEC_MODE_DMA_FENCE)
> +		err = xe_hw_engine_group_suspend_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);

These functions lockdep annotation should be lockdep_assert_held_write too.

> +	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
> + *
> + * 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 mode)

I think you need this annotaion. Unsure if the lock being interruptible
though if this is needed. I'd double check on that.

__acquires(&group->mode_sem);

> +{
> +	int err = down_read_interruptible(&group->mode_sem);
> +
> +	if (err)
> +		return err;
> +
> +	if (mode != group->cur_mode) {
> +		up_read(&group->mode_sem);
> +		err = down_write_killable(&group->mode_sem);
> +		if (err)
> +			return err;
> +
> +		if (mode != group->cur_mode) {
> +			err = switch_mode(group, 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.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index ce59d83a75ad..fce0adf6a7c4 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -73,5 +73,8 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
>  int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
>  int 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 mode);

Maybe consider putting these function in there own files:

xe_hw_engine_group.h
xe_hw_engine_group_types.h
xe_hw_engine_group.c

Matt

> +void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
>  
>  #endif
> -- 
> 2.43.0
>