El mié, 13-05-2026 a las 07:52 -0300, Maíra Canal escribió:
> Hi Iago,
>
> On 13/05/26 02:43, Iago Toral wrote:
> > 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?
>
> If the global perfmon is set before attaching a job->perfmon in the
> submission ioctl, the submission ioctl will fail with -EAGAIN.
>
> If the global perfmon is set after the submission ioctl succeed, but
> before the job actually start the execution, the current job will run
> normally with v3d->global_perfmon.
>
> >
> > 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?
>
> As I write, I have the impression that the best solution would be
> starting the perfmon in the set_global_perfmon ioctl and stop it when
> clearing the global perfmon. Thinking right now, I'm under the
> impression this could simplify the code and ensure a more consistent
> behavior.
>
I was thinking the same, but then we would have to deal with the
situation where we receive a request to set a global perfmon but at the
time we process the ioctl for that we find we already have a non-global
perfmon running. What would you suggest we do in that case?
Iago
> Thanks for your feedback!
>
> Best regards,
> - Maíra
>
> >
> > > >
> > > > 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
> > > >
> > >
> > >
> >
>
>
