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

[Rob Clark]

On Tue, Nov 3, 2026 at 6:24 AM Anna Maniscalco
<anna.maniscalco2000@xxxxxxxxx> wrote:
>
> 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.
>

Yeah, I don't love it either.. but also don't see a better option.  My
best answer is "this is debugfs, it's only best-effort"

BR,
-R