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