El mar, 12-05-2026 a las 07:55 -0300, Maíra Canal escribió:
> Hi Iago,
>
> On 12/05/26 05:20, Iago Toral wrote:
> > El vie, 08-05-2026 a las 10:41 -0300, Maíra Canal escribió:
> > >
> > (...)
> > > +void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon
> > > *perfmon)
> > > +{
> > > + guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
> > > +
> > > + perfmon = v3d->global_perfmon ?: perfmon;
> > > +
> >
> >
> > What's the rationale for having this function do this? The caller
> > explicitly passes a perfmon to be started, but this here can ignore
> > that and instead act on the global perform under the hood.
>
> v3d_perfmon_start() is called in 3 different places: the run_job()
> callback for BIN, RENDER, and CSD jobs. If the global perfmon is
> enabled
> at submission time, we can assure that the job perfmon is NULL.
> However,
> if a global perfmon is set in between the submit ioctl and the job
> actual execution, we give preference to the global perfmon and new
> submissions will be rejected with per-job perfmons while the global
> perfmon is set.
So if we choose the global perfmon here, does that mean the current job
will fail early and won't be submitted?
Also, what would happen if we choose the job's perfmon over the global
perfmon instead? I understand the implication would be that the global
perfmon would have to wait for the next job without a perfmon to be
started but things would otherwise work correctly?
> >
> > I think it would be better if this decision is made expicitly
> > before
> > calling the function and have the function always act on the
> > perfmon
> > requested instead.
> >
>
> I believe this would lead to code duplication as this function is
> called
> in multiple places. Also, this would mean that we would need to
> transfer
> the global perfmon logic to all run_job() callbacks. Also note that
> we
> can only access the v3d->global_perfmon with the spinlock locked, so
> we
> would need to open-code the locking in all run_job() callbacks.
>
Right, we definitely want to avoid that.
Iago
> >
> > > + if (!perfmon)
> > > + return;
> > > +
> > > + if (perfmon == v3d->perfmon_state.active)
> > > + return;
> > > +
> > > + if (!pm_runtime_get_if_active(v3d->drm.dev))
> > > + return;
> > > +
> > > + v3d_perfmon_hw_start(v3d, perfmon);
> > > + v3d->perfmon_state.active = perfmon;
> > > +
> > > + v3d_pm_runtime_put(v3d);
> > > +}
> > > +
> > >
> > (...)
> > >
> > > struct v3d_perfmon *v3d_perfmon_find(struct v3d_file_priv
> > > *v3d_priv,
> > > int id)
> > > @@ -316,11 +354,15 @@ static void v3d_perfmon_delete(struct
> > > v3d_file_priv *v3d_priv,
> > > struct v3d_dev *v3d = v3d_priv->v3d;
> > >
> > > /* If the active perfmon is being destroyed, stop it
> > > first
> > > */
> > > - if (perfmon == v3d->active_perfmon)
> > > - v3d_perfmon_stop(v3d, perfmon, false);
> > > + scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock)
> > > {
> > > + /* If the global perfmon is being destroyed, set
> > > it
> > > to NULL */
> > > + if (v3d->global_perfmon == perfmon) {
> > > + v3d->global_perfmon = NULL;
> > > + v3d_perfmon_put(perfmon);
> > > + }
> > >
> > > - /* If the global perfmon is being destroyed, set it to
> > > NULL
> > > */
> > > - cmpxchg(&v3d->global_perfmon, perfmon, NULL);
> > > + v3d_perfmon_stop_locked(v3d, perfmon, false);
> >
> > I think it would be better to stop the perfmon first and check if
> > it is
> > global second, even if we are certain the put above is never going
> > to
> > delete the last reference in that scenario.
> >
> > Also, maybe we would want to document here with a comment that we
> > increase the refcnt when we set a global perfmon, which is why in
> > that
> > case we put it twice when it is destroyed.
>
> Ack. I'll address it in v2. Thanks for your review!
>
> Best regards,
> - Maíra
>
> >
> > Iago
> >
>
>
