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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > ---
> >   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