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.
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
