Re: [RFC PATCH 1/2] drm/nouveau/pmu/gt215: add graphics engine load counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/nouveau/pmu: non-atomic counter read-and-reset causes PMU cycle loss
--

commit b1f69f3978548eb48a40027083a2b64535b82115
Author: Hamin Sung <hamin@saltyming.net>

drm/nouveau/pmu/gt215: add graphics engine load counters

This commit adds initialization and read functions for the PDAEMON idle
counters on GT215 and related GPUs. The counters track PMU clock cycles and
graphics engine busy cycles, returning and clearing the values when read.

Link: https://envytools.readthedocs.io/en/latest/hw/pm/pdaemon/counter.html

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/pmu/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/pmu/gt215.c
> index 32cee21ed8585..37310a8ebe818 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/pmu/gt215.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/pmu/gt215.c
[ ... ]
> @@ -260,6 +260,44 @@ gt215_pmu_init(struct nvkm_pmu *pmu)
> +static void
> +gt215_pmu_perfmon_read(struct nvkm_pmu *pmu, u32 *busy, u32 *total)
> +{
> +	struct nvkm_device *device = pmu->subdev.device;
> +
> +	*busy = nvkm_rd32(device, 0x10a508 + GT215_PMU_COUNTER_GR * 0x10);
> +	*total = nvkm_rd32(device, 0x10a508 + GT215_PMU_COUNTER_TOTAL * 0x10);
> +	nvkm_wr32(device, 0x10a508 + GT215_PMU_COUNTER_GR * 0x10, 0x80000000);
> +	nvkm_wr32(device, 0x10a508 + GT215_PMU_COUNTER_TOTAL * 0x10, 0x80000000);

[Severity: Medium]
Does this non-atomic counter read-and-reset sequence cause PMU cycle loss?

If a hardware interrupt or CPU preemption occurs between reading the
counters and writing the reset values, any hardware cycles that elapse
during that window are permanently lost from the statistics.

Additionally, a delay between the two reset writes can skew the intervals,
which could result in the busy count exceeding the total count.

A review of the follow-up devfreq patch shows that the caller gracefully
handles the invariant breakage:

if (total && busy <= total)
    ...
else
    stat->busy_time = stat->total_time;

This mitigates the risk of integer underflow or metrics exceeding 100%.
However, it handles the symptom rather than preventing the permanent
loss of PMU cycles during the unprotected preemption window.

> +
> +	*busy &= 0x7fffffff;
> +	*total &= 0x7fffffff;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003223420.77993-1-hamin@saltyming.net?part=1