Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Faking a monotonic timestamp in MSM_PARAM_TIMESTAMP decouples it from hardware command stream timestamps, tracepoints, and performance counters. - [Medium] The monotonic timestamp loses the elapsed time prior to a GPU hang because software reset clears the hardware counter before it can be saved. - [Medium] The accumulated timestamp offset is incorrectly inflated if GPU suspend fails. Pre-existing issues: - [High] Unchecked return value from pm_runtime_get_sync() before accessing hardware registers. -- commit 4aac28caddab346efde440b7be2ebe1a2b708116 Author: Dmitry Baryshkov <[email protected]> drm/msm/adreno: keep the GPU timestamp monotonic across power collapse This patch attempts to make the GPU timestamp reported to userspace monotonic across power collapses. It accumulates the elapsed time during suspend in a software offset and adds it to the hardware counter for the MSM_PARAM_TIMESTAMP ioctl. > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > index 6392126f48f2e..7d9dd9460f5bb 100644 > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > @@ -593,6 +593,8 @@ static int a4xx_pm_suspend(struct msm_gpu *gpu) { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > int ret; > > + adreno_save_timestamp(gpu); > + > ret = msm_gpu_pm_suspend(gpu); > if (ret) > return ret; [Severity: Medium] Will this incorrectly inflate the accumulated timestamp offset if GPU suspend fails? If msm_gpu_pm_suspend() returns an error, the suspend is aborted and the hardware counter is not reset. However, adreno_save_timestamp() has already added the current hardware counter to timestamp_offset. Could this cause the userspace timestamp to jump forward drastically and incorrectly on a failed suspend? (This pattern also applies to the a5xx_pm_suspend() path in this patch.) [ ... ] > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index 3370cd44382fe..f83960b319018 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -226,6 +226,13 @@ adreno_iommu_create_vm(struct msm_gpu *gpu, > return vm; > } > > +void adreno_save_timestamp(struct msm_gpu *gpu) > +{ > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + > + adreno_gpu->timestamp_offset += adreno_gpu->funcs->get_timestamp(gpu); > +} [Severity: Medium] Will the elapsed time prior to a GPU hang be lost because software reset clears the hardware counter before it can be saved? When a GPU hang occurs, recovery routines like a6xx_recover() and a5xx_recover() perform a global software reset to recover the hardware, which clears the hardware counter: /* Reset the GPU to a clean state */ a6xx_gpu_sw_reset(gpu, true); ... pm_runtime_put_sync(&gpu->pdev->dev); When the PM suspend callback later runs and calls adreno_save_timestamp(), it will read a near-zero value. Does this cause the userspace timestamp to jump backwards after a GPU hang? > + > u64 adreno_private_vm_size(struct msm_gpu *gpu) > { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); [ ... ] > @@ -398,7 +405,8 @@ int adreno_get_param(struct msm_gpu *gpu, struct > msm_context *ctx, > case MSM_PARAM_TIMESTAMP: > if (adreno_gpu->funcs->get_timestamp) { > pm_runtime_get_sync(&gpu->pdev->dev); > - *value = adreno_gpu->funcs->get_timestamp(gpu); > + *value = adreno_gpu->timestamp_offset + > + adreno_gpu->funcs->get_timestamp(gpu); > pm_runtime_put_autosuspend(&gpu->pdev->dev); > > return 0; [Severity: High] This is a pre-existing issue, but does the lack of a return value check for pm_runtime_get_sync() lead to a kernel panic if the device fails to resume? If the device fails to resume, reading the memory-mapped timestamp register via get_timestamp() will access an unclocked power domain. On ARM platforms, doesn't this cause a synchronous external abort (kernel panic)? [Severity: High] Does faking a monotonic timestamp here decouple it from hardware command stream timestamps, tracepoints, and performance counters? Userspace relies on MSM_PARAM_TIMESTAMP to calibrate CPU time with GPU hardware time. Because this patch adds a software offset here but cannot add it to timestamps written directly to memory by the GPU (e.g., CP_EVENT_WRITE::TIMESTAMP), the two timestamps become decoupled. Furthermore, internal kernel tracepoints and performance counters still emit the unadjusted timestamp: sample_write_u64(stream, &head, to_adreno_gpu(gpu)->funcs->get_timestamp(gpu)); trace_msm_gpu_submit_flush(submit, adreno_gpu->funcs->get_timestamp(gpu)); Could this break userspace GPU profiling, tracing, and calibration by destroying the correlation between the userspace timestamp and the hardware timestamp? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
