Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Anna Maniscalco]

On 11/3/26 2:58 PM, Rob Clark wrote:
> On Tue, Nov 3, 2026 at 3:28 AM Konrad Dybcio
> <konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
> > On 11/2/26 8:00 PM, Rob Clark wrote:
> > > We need to move the gpu lock around a bit for the next patch.  Use
> > > scoped_guard/etc so we don't have to make the error path handling more
> > > complicated.
> > >
> > > Signed-off-by: Rob Clark <robin.clark@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/msm/msm_debugfs.c | 31 ++++++++++++-------------------
> > >   1 file changed, 12 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> > > index f12701e286ec..aa59b28a3392 100644
> > > --- a/drivers/gpu/drm/msm/msm_debugfs.c
> > > +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> > > @@ -70,43 +70,36 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
> > >        struct drm_device *dev = inode->i_private;
> > >        struct msm_drm_private *priv = dev->dev_private;
> > >        struct msm_gpu *gpu = priv->gpu;
> > > -     struct msm_gpu_show_priv *show_priv;
> > >        int ret;
> > >
> > >        if (!gpu || !gpu->funcs->gpu_state_get)
> > >                return -ENODEV;
> > >
> > > -     show_priv = kmalloc_obj(*show_priv);
> > > +     struct msm_gpu_show_priv *show_priv __free(kfree)
> > > +             = kmalloc_obj(*show_priv);
> > >        if (!show_priv)
> > >                return -ENOMEM;
> > >
> > > -     ret = mutex_lock_interruptible(&gpu->lock);
> > > -     if (ret)
> > > -             goto free_priv;
> > > -
> > > -     pm_runtime_get_sync(&gpu->pdev->dev);
> > > -     msm_gpu_hw_init(gpu);
> > > -     show_priv->state = gpu->funcs->gpu_state_get(gpu);
> > > -     pm_runtime_put_sync(&gpu->pdev->dev);
> > > -
> > > -     mutex_unlock(&gpu->lock);
> > > +     scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> > > +             scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
> > > +                     msm_gpu_hw_init(gpu);
> > > +                     show_priv->state = gpu->funcs->gpu_state_get(gpu);
> > > +             }
> > My clanker tells me we can't grab the PM reference before a mutex,
> > as the recovery mechanism depends on the rpm count being appropriate
> > to let it drop it (and kill gx this way)
> Yeah, but we also can't do it in the other order, or more specifically
> disabling ifpc needs to come before gpu lock or it can deadlock with
> recovery.. but ifpc disable needs to be after runpm get.
>
> Re-working that ordering was the whole of introducing cleanup.h, to
> avoid the error path with mutex_lock_interruptible() after taking
> runpm
>
> I guess if you are hammering on debugfs while crashing the gpu, maybe
> the first recovery fails, and then a subsequent one eventually
> succeeds.
I guess it leads to a sort of deadlock scenario because the recovery 
path waits
for power to collapse, though it timeouts after one second.

After that hangcheck will have to trigger again so I guess the delay 
introduced
would be significant.

I really wish there was some better way of doing recovery that didn't 
rely on
the counbter having a known value.

> BR,
> -R


Best regards,
--
Anna Maniscalco <anna.maniscalco2000@xxxxxxxxx>