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? > Thanks, > Steve
