On 06/10/2026 16:09, Adrián Larumbe wrote: > On 2026-10-02 15:59:20+01:00, Steven Price wrote: >> On 29/09/2026 04:44, Adrián Larumbe wrote: >> >>> Rather than just failing silently, let's warn the user of device remove not >>> being able to take an PM reference or the PM suspend path still reporting >>> inflight jobs. Neither situation should ever happen. >>> >>> Reviewed-by: Boris Brezillon <[email protected]> >>> Signed-off-by: Adrián Larumbe <[email protected]> >>> --- >>> drivers/gpu/drm/panfrost/panfrost_device.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c >>> b/drivers/gpu/drm/panfrost/panfrost_device.c >>> index c6bf3d0663df..09a5752a3f40 100644 >>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c >>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c >>> @@ -9,6 +9,7 @@ >>> #include <linux/pm_runtime.h> >>> #include <linux/regulator/consumer.h> >>> #include <drm/drm_drv.h> >>> +#include <drm/drm_print.h> >>> >>> #include "panfrost_device.h" >>> #include "panfrost_devfreq.h" >>> @@ -357,7 +358,7 @@ int panfrost_device_init(struct panfrost_device *pfdev) >>> >>> void panfrost_device_fini(struct panfrost_device *pfdev) >>> { >>> - pm_runtime_get_sync(pfdev->base.dev); >>> + drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0); >> >> This seems fine. >> >>> pm_runtime_dont_use_autosuspend(pfdev->base.dev); >>> pm_runtime_disable(pfdev->base.dev); >>> @@ -516,7 +517,7 @@ static int panfrost_device_runtime_suspend(struct >>> device *dev) >>> { >>> struct panfrost_device *pfdev = dev_get_drvdata(dev); >>> >>> - if (!panfrost_jm_is_idle(pfdev)) >>> + if (drm_WARN_ON(&pfdev->base, !panfrost_jm_is_idle(pfdev))) >> >> I'm a bit wary that this might be something that user space can trigger. >> My AI says: >> >> The runtime-suspend WARN can be reached by ordinary userspace job >> submissions. The DRM scheduler increments credit_count before calling >> Panfrost’s job runner (drivers/gpu/drm/scheduler/sched_main.c:1044). >> Panfrost takes the job’s PM reference later in hardware submission >> (drivers/gpu/drm/panfrost/panfrost_job.c:213). If autosuspend runs in >> that interval, the new WARN >> (drivers/gpu/drm/panfrost/panfrost_device.c:525) sees the credit and >> fires, even though this is a timing race rather than a broken job. >> Repeated submissions near the autosuspend boundary could therefore >> produce repeated stack traces. The PM core treats the resulting -EBUSY >> as a transient failure. >> >> Now I have to admit I don't trust it that much - but I'd want a >> convincing argument on why panfrost_jm_is_idle() will never be false here. > > You're right. I was in the belief that autosuspend kicking in was proof of no > inflight or pending jobs present in the scheduler queues, so I came to treat > this check as things having gone awry. > > I guess its value lies in the ability of the PM runtime suspend handler > to cancel itself at an autosuspend event, like you said. > > However, it just made me wonder: what would happen in the event that > autosuspend > kicks in and runs panfrost_device_runtime_suspend() right at the same time > that > a scheduler job is picked up by drm_sched_run_job_work(), but hasn't yet > reached > the statement where it does an atomic increment on the credit_count? I guess > nothing, because panfrost_job_hw_submit() is getting a PM reference before > accessing any HW registers, and that should take care of dealing with any > ongoing autosuspend events. > > In that case I'll just delete that warning. However, I'd say it's bad practice > to have DRM drivers access the internal state of the DRM scheduler. At > present, > only Panfrost and Etnaviv poke it in their RPM suspend handlers, and I've been > wondering whether we should get rid of this check altogether, or else maybe > ask > the scheduler maintainers whether it makes sense to have a non-racy way to > query the presence of pending jobs in their queues?
Yes, I'm not sure whether we actually need that check. As you say there's still a race where panfrost_jm_is_idle() returns true, but afterwards credit_count is incremented. I think this is safe - panfrost_job_hw_submit() takes a PM reference which will cause the GPU to be woken up again. So we could just drop the panfrost_jm_is_idle() function completely. Whether that has any performance impact - i.e. do we often race the autosuspend operation? - I've no idea. Presumably you didn't hit it when you had the WARN in place. So if you'd prefer to just remove the code then that's fine by me. Thanks, Steve
