Re: [PATCH v12 09/15] drm/panfrost: Add warning messages to fatal error conditions

[Steven Price <steven.price@arm.com>]

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 <boris.brezillon@collabora.com>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>>> ---
>>>  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