Re: [PATCH 1/5] drm/v3d: Don't increment `enabled_ns` twice

2024-04-16 Thread Tvrtko Ursulin 



Hi,

On 15/04/2024 12:17, Chema Casanova wrote:

El 3/4/24 a las 22:24, Maíra Canal escribió:
The commit 509433d8146c ("drm/v3d: Expose the total GPU usage stats on 
sysfs")

introduced the calculation of global GPU stats. For the regards, it used
the already existing infrastructure provided by commit 09a93cc4f7d1 
("drm/v3d:

Implement show_fdinfo() callback for GPU usage stats"). While adding
global GPU stats calculation ability, the author forgot to delete the
existing one.

Currently, the value of `enabled_ns` is incremented twice by the end of
the job, when it should be added just once. Therefore, delete the
leftovers from commit 509433d8146c ("drm/v3d: Expose the total GPU usage
stats on sysfs").

Fixes: 509433d8146c ("drm/v3d: Expose the total GPU usage stats on 
sysfs")

Reported-by: Tvrtko Ursulin 
Signed-off-by: Maíra Canal 
---
  drivers/gpu/drm/v3d/v3d_irq.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c 
b/drivers/gpu/drm/v3d/v3d_irq.c

index 2e04f6cb661e..ce6b2fb341d1 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -105,7 +105,6 @@ v3d_irq(int irq, void *arg)
  struct v3d_file_priv *file = 
v3d->bin_job->base.file->driver_priv;

  u64 runtime = local_clock() - file->start_ns[V3D_BIN];
-    file->enabled_ns[V3D_BIN] += local_clock() - 
file->start_ns[V3D_BIN];

  file->jobs_sent[V3D_BIN]++;
  v3d->queue[V3D_BIN].jobs_sent++;
@@ -126,7 +125,6 @@ v3d_irq(int irq, void *arg)
  struct v3d_file_priv *file = 
v3d->render_job->base.file->driver_priv;

  u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
-    file->enabled_ns[V3D_RENDER] += local_clock() - 
file->start_ns[V3D_RENDER];

  file->jobs_sent[V3D_RENDER]++;
  v3d->queue[V3D_RENDER].jobs_sent++;
@@ -147,7 +145,6 @@ v3d_irq(int irq, void *arg)
  struct v3d_file_priv *file = 
v3d->csd_job->base.file->driver_priv;

  u64 runtime = local_clock() - file->start_ns[V3D_CSD];
-    file->enabled_ns[V3D_CSD] += local_clock() - 
file->start_ns[V3D_CSD];

  file->jobs_sent[V3D_CSD]++;
  v3d->queue[V3D_CSD].jobs_sent++;
@@ -195,7 +192,6 @@ v3d_hub_irq(int irq, void *arg)
  struct v3d_file_priv *file = 
v3d->tfu_job->base.file->driver_priv;

  u64 runtime = local_clock() - file->start_ns[V3D_TFU];
-    file->enabled_ns[V3D_TFU] += local_clock() - 
file->start_ns[V3D_TFU];

  file->jobs_sent[V3D_TFU]++;
  v3d->queue[V3D_TFU].jobs_sent++;


Thanks for fixing this. I see that I included this error in my first 
refactoring of

the original patch.


Not sure if it would be worth creating a simple test like 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/2f81ed3aed873c7cc2f6d0e1117fa4fb02033246 
for i915? Just a thought.


Regards,

Tvrtko

Re: [PATCH 1/5] drm/v3d: Don't increment `enabled_ns` twice

2024-04-15 Thread Maíra Canal 

On 4/3/24 17:24, Maíra Canal wrote:

The commit 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
introduced the calculation of global GPU stats. For the regards, it used
the already existing infrastructure provided by commit 09a93cc4f7d1 ("drm/v3d:
Implement show_fdinfo() callback for GPU usage stats"). While adding
global GPU stats calculation ability, the author forgot to delete the
existing one.

Currently, the value of `enabled_ns` is incremented twice by the end of
the job, when it should be added just once. Therefore, delete the
leftovers from commit 509433d8146c ("drm/v3d: Expose the total GPU usage
stats on sysfs").

Fixes: 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
Reported-by: Tvrtko Ursulin 
Signed-off-by: Maíra Canal 


As this patch is a isolated bugfix and it was reviewed by two
developers, I'm applying it to drm-misc/drm-misc-fixes.

I'll address the feedback for the rest of the series later and send a
v2.

Best Regards,
- Maíra


---
  drivers/gpu/drm/v3d/v3d_irq.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 2e04f6cb661e..ce6b2fb341d1 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -105,7 +105,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->bin_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_BIN];
  
-		file->enabled_ns[V3D_BIN] += local_clock() - file->start_ns[V3D_BIN];

file->jobs_sent[V3D_BIN]++;
v3d->queue[V3D_BIN].jobs_sent++;
  
@@ -126,7 +125,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->render_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
  
-		file->enabled_ns[V3D_RENDER] += local_clock() - file->start_ns[V3D_RENDER];

file->jobs_sent[V3D_RENDER]++;
v3d->queue[V3D_RENDER].jobs_sent++;
  
@@ -147,7 +145,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->csd_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_CSD];
  
-		file->enabled_ns[V3D_CSD] += local_clock() - file->start_ns[V3D_CSD];

file->jobs_sent[V3D_CSD]++;
v3d->queue[V3D_CSD].jobs_sent++;
  
@@ -195,7 +192,6 @@ v3d_hub_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->tfu_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_TFU];
  
-		file->enabled_ns[V3D_TFU] += local_clock() - file->start_ns[V3D_TFU];

file->jobs_sent[V3D_TFU]++;
v3d->queue[V3D_TFU].jobs_sent++;

Re: [PATCH 1/5] drm/v3d: Don't increment `enabled_ns` twice

2024-04-15 Thread Chema Casanova 

El 3/4/24 a las 22:24, Maíra Canal escribió:

The commit 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
introduced the calculation of global GPU stats. For the regards, it used
the already existing infrastructure provided by commit 09a93cc4f7d1 ("drm/v3d:
Implement show_fdinfo() callback for GPU usage stats"). While adding
global GPU stats calculation ability, the author forgot to delete the
existing one.

Currently, the value of `enabled_ns` is incremented twice by the end of
the job, when it should be added just once. Therefore, delete the
leftovers from commit 509433d8146c ("drm/v3d: Expose the total GPU usage
stats on sysfs").

Fixes: 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
Reported-by: Tvrtko Ursulin 
Signed-off-by: Maíra Canal 
---
  drivers/gpu/drm/v3d/v3d_irq.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 2e04f6cb661e..ce6b2fb341d1 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -105,7 +105,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->bin_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_BIN];
  
-		file->enabled_ns[V3D_BIN] += local_clock() - file->start_ns[V3D_BIN];

file->jobs_sent[V3D_BIN]++;
v3d->queue[V3D_BIN].jobs_sent++;
  
@@ -126,7 +125,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->render_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
  
-		file->enabled_ns[V3D_RENDER] += local_clock() - file->start_ns[V3D_RENDER];

file->jobs_sent[V3D_RENDER]++;
v3d->queue[V3D_RENDER].jobs_sent++;
  
@@ -147,7 +145,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->csd_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_CSD];
  
-		file->enabled_ns[V3D_CSD] += local_clock() - file->start_ns[V3D_CSD];

file->jobs_sent[V3D_CSD]++;
v3d->queue[V3D_CSD].jobs_sent++;
  
@@ -195,7 +192,6 @@ v3d_hub_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->tfu_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_TFU];
  
-		file->enabled_ns[V3D_TFU] += local_clock() - file->start_ns[V3D_TFU];

file->jobs_sent[V3D_TFU]++;
v3d->queue[V3D_TFU].jobs_sent++;
  


Thanks for fixing this. I see that I included this error in my first 
refactoring of

the original patch.

Reviewed-by: Jose Maria Casanova Crespo

Re: [PATCH 1/5] drm/v3d: Don't increment `enabled_ns` twice

2024-04-15 Thread Tvrtko Ursulin 



On 03/04/2024 21:24, Maíra Canal wrote:

The commit 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
introduced the calculation of global GPU stats. For the regards, it used
the already existing infrastructure provided by commit 09a93cc4f7d1 ("drm/v3d:
Implement show_fdinfo() callback for GPU usage stats"). While adding
global GPU stats calculation ability, the author forgot to delete the
existing one.

Currently, the value of `enabled_ns` is incremented twice by the end of
the job, when it should be added just once. Therefore, delete the
leftovers from commit 509433d8146c ("drm/v3d: Expose the total GPU usage
stats on sysfs").

Fixes: 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
Reported-by: Tvrtko Ursulin 
Signed-off-by: Maíra Canal 
---
  drivers/gpu/drm/v3d/v3d_irq.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 2e04f6cb661e..ce6b2fb341d1 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -105,7 +105,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->bin_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_BIN];
  
-		file->enabled_ns[V3D_BIN] += local_clock() - file->start_ns[V3D_BIN];

file->jobs_sent[V3D_BIN]++;
v3d->queue[V3D_BIN].jobs_sent++;
  
@@ -126,7 +125,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->render_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
  
-		file->enabled_ns[V3D_RENDER] += local_clock() - file->start_ns[V3D_RENDER];

file->jobs_sent[V3D_RENDER]++;
v3d->queue[V3D_RENDER].jobs_sent++;
  
@@ -147,7 +145,6 @@ v3d_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->csd_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_CSD];
  
-		file->enabled_ns[V3D_CSD] += local_clock() - file->start_ns[V3D_CSD];

file->jobs_sent[V3D_CSD]++;
v3d->queue[V3D_CSD].jobs_sent++;
  
@@ -195,7 +192,6 @@ v3d_hub_irq(int irq, void *arg)

struct v3d_file_priv *file = 
v3d->tfu_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_TFU];
  
-		file->enabled_ns[V3D_TFU] += local_clock() - file->start_ns[V3D_TFU];

file->jobs_sent[V3D_TFU]++;
v3d->queue[V3D_TFU].jobs_sent++;
  


Reviewed-by: Tvrtko Ursulin 

Regards,

Tvrtko

[PATCH 1/5] drm/v3d: Don't increment `enabled_ns` twice

2024-04-03 Thread Maíra Canal 
The commit 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
introduced the calculation of global GPU stats. For the regards, it used
the already existing infrastructure provided by commit 09a93cc4f7d1 ("drm/v3d:
Implement show_fdinfo() callback for GPU usage stats"). While adding
global GPU stats calculation ability, the author forgot to delete the
existing one.

Currently, the value of `enabled_ns` is incremented twice by the end of
the job, when it should be added just once. Therefore, delete the
leftovers from commit 509433d8146c ("drm/v3d: Expose the total GPU usage
stats on sysfs").

Fixes: 509433d8146c ("drm/v3d: Expose the total GPU usage stats on sysfs")
Reported-by: Tvrtko Ursulin 
Signed-off-by: Maíra Canal 
---
 drivers/gpu/drm/v3d/v3d_irq.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 2e04f6cb661e..ce6b2fb341d1 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -105,7 +105,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->bin_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_BIN];
 
-   file->enabled_ns[V3D_BIN] += local_clock() - 
file->start_ns[V3D_BIN];
file->jobs_sent[V3D_BIN]++;
v3d->queue[V3D_BIN].jobs_sent++;
 
@@ -126,7 +125,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->render_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
 
-   file->enabled_ns[V3D_RENDER] += local_clock() - 
file->start_ns[V3D_RENDER];
file->jobs_sent[V3D_RENDER]++;
v3d->queue[V3D_RENDER].jobs_sent++;
 
@@ -147,7 +145,6 @@ v3d_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->csd_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_CSD];
 
-   file->enabled_ns[V3D_CSD] += local_clock() - 
file->start_ns[V3D_CSD];
file->jobs_sent[V3D_CSD]++;
v3d->queue[V3D_CSD].jobs_sent++;
 
@@ -195,7 +192,6 @@ v3d_hub_irq(int irq, void *arg)
struct v3d_file_priv *file = 
v3d->tfu_job->base.file->driver_priv;
u64 runtime = local_clock() - file->start_ns[V3D_TFU];
 
-   file->enabled_ns[V3D_TFU] += local_clock() - 
file->start_ns[V3D_TFU];
file->jobs_sent[V3D_TFU]++;
v3d->queue[V3D_TFU].jobs_sent++;
 
-- 
2.44.0