El mié, 13-05-2026 a las 08:08 -0300, Maíra Canal escribió:
>
>
> On 13/05/26 08:01, Iago Toral wrote:
> > 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?
>
> In this series, I'm proposing returning -EBUSY if v3d-
> >perfmon_state.active
> is already set. However, even if we don't do that, I don't believe
> this
> would be an issue, as v3d_perfmon_start() is guarded by the spinlock.
>
Returning -EBUSY sounds good to me.
Iago
> Best regards,
> - Maíra
>
> >
> > 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
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> >
>
>
