Re: [PATCH v9 10/16] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

["Adrián Larumbe" <adrian.larumbe@collabora.com>]

> On 23.09.2026 09:10, Boris Brezillon wrote:
> On Tue, 22 Sep 2026 20:54:11 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > On 14.09.2026 11:39, Boris Brezillon wrote:
> > > On Sat, 12 Sep 2026 00:28:11 +0100
> > > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> > >   
> > > > This will be of great help when testing potential races between the GPU
> > > > reset sequence and other parts of the code accessing HW registers.
> > > > 
> > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>  
> > > 
> > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > 
> > > One comment below.
> > >   
> > > > ---
> > > >  drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 37 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > > index d75566a45392..0aee1d87a7ab 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > > @@ -2,6 +2,7 @@
> > > >  /* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
> > > >  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> > > >  
> > > > +#include <linux/debugfs.h>
> > > >  #include <linux/clk.h>
> > > >  #include <linux/reset.h>
> > > >  #include <linux/platform_device.h>
> > > > @@ -583,9 +584,45 @@ EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
> > > >  };
> > > >  
> > > >  #ifdef CONFIG_DEBUG_FS
> > > > +static int reset_get(void *data, u64 *val)
> > > > +{
> > > > +	struct panfrost_device *pfdev =
> > > > +		container_of(data, struct panfrost_device, base);
> > > > +
> > > > +	*val = atomic_read(&pfdev->reset.pending);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int reset_set(void *data, u64 val)
> > > > +{
> > > > +	struct panfrost_device *pfdev =
> > > > +		container_of(data, struct panfrost_device, base);
> > > > +	int ret = pm_runtime_get_if_active(pfdev->base.dev);
> > > > +
> > > > +	if (ret) {
> > > > +		panfrost_device_schedule_reset(pfdev);  
> > > 
> > > It feels weird to schedule a reset if the device is suspended, because
> > > a suspend/resume cycle already acts as a reset. Any particular reason
> > > you'd want to do that? If not, I'd suggest going for:  
> > 
> > That's been my goal all along, for the knob to trigger a reset only when
> > the device isn't suspended. pm_runtime_get_if_active() docs states that:
> > 
> > """
> > Increment the runtime PM usage counter of @dev if its runtime PM status is
> > %RPM_ACTIVE, in which case it returns 1. If the device is in a different
> > state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
> > device, in which case also the usage_count will remain unmodified.
> > """
> > 
> > So (ret != 0) means the device either doesn't support RPM or else it
> > was in the ACTIVE state. In both cases, that means the device is powered
> > so we can proceed with the reset. However, I only decrement the usage
> > counter when kernel RPM is enabled (ret > 1).
> 
> Okay, I see. Can we have a comment explaining that? Also, I think I
> prefer an early bail out over the extra indentation level added here.
> 
> 	if (!ret)
> 		return 0;

Will do.

> Oh, and we should probably make this knob inactive in the device removal
> path (with a disable_work()?), otherwise -EINVAL will be returned as
> soon as pm_runtime_disable() is called, and this might cause a reset to
> be scheduled, which we probably don't want, since it would interfere
> with our device_fini() logic.

I think replacing cancel_work_sync() with disable_work_sync() in panfrost_jm_fini()
should be enough to this end. In that case, both queue_work() and flush_work()
will do nothing and no PM reference will be taken, so the whole thing effective
becomes a NOP.

> > 
> > > 	if (ret <= 0)
> > > 		return ret;
> > > 
> > > 	panfrost_device_schedule_reset(pfdev);
> > > 	pm_runtime_put(pfdev->base.dev);
> > > 	return 0;
> > >   
> > > > +		flush_work(&pfdev->reset.work);
> > > > +		if (ret > 0)
> > > > +			pm_runtime_put(pfdev->base.dev);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
> > > > +			 reset_get, reset_set,
> > > > +			 "0x%08llx\n");
> > > > +
> > > > +static void panfrost_reset_debugfs_init(struct drm_minor *minor)
> > > > +{
> > > > +	debugfs_create_file("reset", 0600, minor->debugfs_root,
> > > > +			    minor->dev, &panfrost_reset_debugfs_fops);
> > > > +}
> > > > +
> > > >  void panfrost_device_debugfs_init(struct drm_minor *minor)
> > > >  {
> > > >  	panfrost_gem_debugfs_init(minor);
> > > >  	panfrost_sched_debugfs_init(minor);
> > > > +	panfrost_reset_debugfs_init(minor);
> > > >  }
> > > >  #endif // CONFIG_DEBUG_FS
> > > >   
> > 
> > Adrian Larumbe


Adrian Larumbe