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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Jun 27, 2024 at 03:24:23PM -0500, Lucas De Marchi wrote:
>On Fri, Jun 28, 2024 at 01:23:47AM GMT, 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.
>>
>>v2: Drop the Fixes tag
>>
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
>
>Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>I think the names are not very good for what this is doing now.  It's
>not clear *_delta_xxxx  will flush the previous accumulated values.
>
>Maybe it'd be better to have:
>
>	xe_exec_queue_accumulate_run_ticks();
>	xef->run_ticks[q->class] += xe_exec_queue_flush_run_ticks(q);
>
>So names are clearer and you don't have a function that does 2 things.

Sure, I will rename them.

Regards,
Umesh
>
>Lucas De Marchi
>
>>---
>>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
>>