| Message ID | 20240403203517.731876-2-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: Fix GPU stats inconsistancies and race-condition | expand |
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 <tursulin@igalia.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > 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 <tvrtko.ursulin@igalia.com> Regards, Tvrtko
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 <tursulin@igalia.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > 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 <jmcasanova@igalia.com>
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 <tursulin@igalia.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> 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++; >
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 <tursulin@igalia.com> >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> 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
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++;
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 <tursulin@igalia.com> Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/v3d/v3d_irq.c | 4 ---- 1 file changed, 4 deletions(-)