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

[Anna Maniscalco <anna.maniscalco2000@gmail.com>]

On 11/3/26 2:58 PM, Rob Clark wrote:
> On Tue, Nov 3, 2026 at 3:28 AM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> 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@oss.qualcomm.com>
>>> ---
>>>   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@gmail.com>