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 01:31:42PM -0700, Umesh Nerlige Ramappa wrote:
>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.

I think I will hold off on this patch since I need to add the reference 
counting for xef object. Once I do that, this may not be needed.

Thanks,
Umesh
>
>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
>>>