| Message ID | 20240403203517.731876-5-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: > Given a set of GPU stats, that is, a `struct v3d_stats` related to a > queue in a given context, create a function that can update all this set of > GPU stats. > > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > drivers/gpu/drm/v3d/v3d_sched.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index ea5f5a84b55b..754107b80f67 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -118,6 +118,16 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) > global_stats->start_ns = now; > } > > +static void > +v3d_stats_update(struct v3d_stats *stats) > +{ > + u64 now = local_clock(); > + > + stats->enabled_ns += now - stats->start_ns; > + stats->jobs_sent++; > + stats->start_ns = 0; > +} > + > void > v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) > { > @@ -125,15 +135,9 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) > struct v3d_file_priv *file = job->file->driver_priv; > struct v3d_stats *global_stats = &v3d->queue[queue].stats; > struct v3d_stats *local_stats = &file->stats[queue]; > - u64 now = local_clock(); > - > - local_stats->enabled_ns += now - local_stats->start_ns; > - local_stats->jobs_sent++; > - local_stats->start_ns = 0; > > - global_stats->enabled_ns += now - global_stats->start_ns; > - global_stats->jobs_sent++; > - global_stats->start_ns = 0; > + v3d_stats_update(local_stats); > + v3d_stats_update(global_stats); > } > > static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Regards, Tvrtko
El 3/4/24 a las 22:24, Maíra Canal escribió: > Given a set of GPU stats, that is, a `struct v3d_stats` related to a > queue in a given context, create a function that can update all this set of > GPU stats. > > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > drivers/gpu/drm/v3d/v3d_sched.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index ea5f5a84b55b..754107b80f67 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -118,6 +118,16 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) > global_stats->start_ns = now; > } > > +static void > +v3d_stats_update(struct v3d_stats *stats) > +{ > + u64 now = local_clock(); I understand that with this change, we would be calling twice local_clock() for each stat update, once for local and one for global stats. I don't know the performance impact of the extra local_clock(), but I understand as you are always updating global_stats after local_stats we would be losing the correspondence that global_stats is the sum of all local_stats for all process. With this approach, it will be always greater or equal. Maybe it makes sense to pass an extra parameter now so save one local_clock() and the sum of global and local stats is the same when you only have one process running. > + > + stats->enabled_ns += now - stats->start_ns; > + stats->jobs_sent++; > + stats->start_ns = 0; > +} > + > void > v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) > { > @@ -125,15 +135,9 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) > struct v3d_file_priv *file = job->file->driver_priv; > struct v3d_stats *global_stats = &v3d->queue[queue].stats; > struct v3d_stats *local_stats = &file->stats[queue]; > - u64 now = local_clock(); > - > - local_stats->enabled_ns += now - local_stats->start_ns; > - local_stats->jobs_sent++; > - local_stats->start_ns = 0; > > - global_stats->enabled_ns += now - global_stats->start_ns; > - global_stats->jobs_sent++; > - global_stats->start_ns = 0; > + v3d_stats_update(local_stats); > + v3d_stats_update(global_stats); > } > > static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index ea5f5a84b55b..754107b80f67 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -118,6 +118,16 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) global_stats->start_ns = now; } +static void +v3d_stats_update(struct v3d_stats *stats) +{ + u64 now = local_clock(); + + stats->enabled_ns += now - stats->start_ns; + stats->jobs_sent++; + stats->start_ns = 0; +} + void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) { @@ -125,15 +135,9 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) struct v3d_file_priv *file = job->file->driver_priv; struct v3d_stats *global_stats = &v3d->queue[queue].stats; struct v3d_stats *local_stats = &file->stats[queue]; - u64 now = local_clock(); - - local_stats->enabled_ns += now - local_stats->start_ns; - local_stats->jobs_sent++; - local_stats->start_ns = 0; - global_stats->enabled_ns += now - global_stats->start_ns; - global_stats->jobs_sent++; - global_stats->start_ns = 0; + v3d_stats_update(local_stats); + v3d_stats_update(global_stats); } static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
Given a set of GPU stats, that is, a `struct v3d_stats` related to a queue in a given context, create a function that can update all this set of GPU stats. Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/v3d/v3d_sched.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)