Re: [PATCH 1/2] drm/xe: Use a helper to get delta run ticks from the exec queue

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

On Wed, Jun 26, 2024 at 12:58:11AM +0800, Umesh Nerlige Ramappa wrote:
> As per earlier review feedback - code manipulating exec queue fields
> should not be present in xe_device.c. Use a helper to get the delta run
> ticks from exec queue.
> 
> Fixes: ce62827bc294 ("drm/xe: Do not access xe file when updating exec queue run_ticks")

I'm not sure if fixes applies here this is not changing behavior. Maybe
ask a maintainer for clarification here.

Patch itself is a good cleanup and looks correct.

With a clarification on the fixes tag usage:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_drm_client.c |  7 ++-----
>  drivers/gpu/drm/xe/xe_exec_queue.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/xe/xe_exec_queue.h |  1 +
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
> index 4a19b771e3a0..e0c4a50d372c 100644
> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> @@ -251,11 +251,8 @@ static void show_run_ticks(struct drm_printer *p, struct drm_file *file)
>  
>  	/* Accumulate all the exec queues from this client */
>  	mutex_lock(&xef->exec_queue.lock);
> -	xa_for_each(&xef->exec_queue.xa, i, q) {
> -		xe_exec_queue_update_run_ticks(q);
> -		xef->run_ticks[q->class] += q->run_ticks - q->old_run_ticks;
> -		q->old_run_ticks = q->run_ticks;
> -	}
> +	xa_for_each(&xef->exec_queue.xa, i, q)
> +		xef->run_ticks[q->class] += xe_exec_queue_delta_run_ticks(q);
>  	mutex_unlock(&xef->exec_queue.lock);
>  
>  	/* Get the total GPU cycles */
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 0ba37835849b..4d90a16745d2 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -786,6 +786,24 @@ void xe_exec_queue_update_run_ticks(struct xe_exec_queue *q)
>  	q->run_ticks += (new_ts - old_ts) * q->width;
>  }
>  
> +/**
> + * xe_exec_queue_delta_run_ticks() - Get delta of queue run_ticks 
> + * @q: The exec queue
> + *
> + * Update run ticks for the specific queue and then return the delta when
> + * compared to the previous value of run ticks.
> + */
> +u64 xe_exec_queue_delta_run_ticks(struct xe_exec_queue *q)
> +{
> +	u64 delta_ticks;
> +
> +	xe_exec_queue_update_run_ticks(q);
> +	delta_ticks = q->run_ticks - q->old_run_ticks;
> +	q->old_run_ticks = q->run_ticks;
> +
> +	return delta_ticks;
> +}
> +
>  void xe_exec_queue_kill(struct xe_exec_queue *q)
>  {
>  	struct xe_exec_queue *eq = q, *next;
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
> index 289a3a51d2a2..42f683e5d88a 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> @@ -76,5 +76,6 @@ struct dma_fence *xe_exec_queue_last_fence_get(struct xe_exec_queue *e,
>  void xe_exec_queue_last_fence_set(struct xe_exec_queue *e, struct xe_vm *vm,
>  				  struct dma_fence *fence);
>  void xe_exec_queue_update_run_ticks(struct xe_exec_queue *q);
> +u64 xe_exec_queue_delta_run_ticks(struct xe_exec_queue *q);
>  
>  #endif
> -- 
> 2.34.1
>