Hi Tvrtko,
On 07/08/26 05:54, Tvrtko Ursulin wrote:
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/
vc4_gem.c
index f0c711f114cf..f84d82b3bfbb 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -491,7 +491,7 @@ vc4_submit_next_bin_job(struct drm_device *dev)
/* Only start the perfmon if it was not already started by a
previous
* job.
*/
- if (exec->perfmon && vc4->active_perfmon != exec->perfmon)
+ if (exec->perfmon && vc4->perfmon_state.active != exec->perfmon)
Shouldn't the second check be moved to under the lock in vc4_perfmon_start?
Yeah, you are correct. As this check won't be needed in a future patch,
I believe I forgot it. I'll add it to this patch in the next version to
guarantee bisectability.
vc4_perfmon_start(vc4, exec->perfmon);
/* Either put the job in the binner if it uses the binner, or
@@ -1138,6 +1138,7 @@ int vc4_gem_init(struct drm_device *dev)
INIT_LIST_HEAD(&vc4->render_job_list);
INIT_LIST_HEAD(&vc4->job_done_list);
spin_lock_init(&vc4->job_lock);
+ spin_lock_init(&vc4->perfmon_state.lock);
INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
timer_setup(&vc4->hangcheck.timer, vc4_hangcheck_elapsed, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c b/drivers/gpu/drm/vc4/
vc4_perfmon.c
index f75dfd156756..2bcf6a7d538f 100644
--- a/drivers/gpu/drm/vc4/vc4_perfmon.c
+++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
@@ -51,16 +51,18 @@ void vc4_perfmon_start(struct vc4_dev *vc4, struct
vc4_perfmon *perfmon)
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
- if (WARN_ON_ONCE(!perfmon || vc4->active_perfmon))
+ if (!perfmon)
return;
The only caller already ensures perfmon cannot be NULL so the check here
is redundant. Maybe you want it for later in the series, or for symmetry
with stop? Will see later.
What is lost though is the second check, especially since
vc4_submit_next_bin_job has its "active differs" check outside the lock.
Was that an impossible condition to begin with? Is there scope to clean
something up before adding a new lock in this patch?
+ guard(spinlock_irqsave)(&vc4->perfmon_state.lock);
+
for (i = 0; i < perfmon->ncounters; i++)
V3D_WRITE(V3D_PCTRS(i), perfmon->events[i]);
mask = GENMASK(perfmon->ncounters - 1, 0);
V3D_WRITE(V3D_PCTRC, mask);
V3D_WRITE(V3D_PCTRE, V3D_PCTRE_EN | mask);
These are mmio? How costly on this hardware are they? Lets see later in
the series if they can be moved outside the locked irq off section by
maybe courtesy of run_job implicit serialisation.
I believe we still need to guard them against the delete IOCTL and fd
closure.
Hm, does it even need to be irqsave ie can this be called from interrupt
context?
In a couple of patches, it will :)
- vc4->active_perfmon = perfmon;
+ vc4->perfmon_state.active = perfmon;
}
void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon,
@@ -71,8 +73,12 @@ void vc4_perfmon_stop(struct vc4_dev *vc4, struct
vc4_perfmon *perfmon,
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
- if (WARN_ON_ONCE(!vc4->active_perfmon ||
- perfmon != vc4->active_perfmon))
Not worth keeping the "unexpected perfmon" warning in some fashion?
I have the impression that vc4_perfmon_delete() can use the return "if
(perfmon != vc4->perfmon_state.active)" in a legitimate way, that's why
I removed the warning.
Best regards,
- Maíra
