Re: [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 31.08.21 um 07:47 schrieb Pan, Xinhui:
>
> 在 2021/8/31 13:38，“Pan, Xinhui”<Xinhui.Pan@amd.com> 写入:
>
>      
>      
>      在 2021/8/31 12:03，“Grodzovsky, Andrey”<Andrey.Grodzovsky@amd.com> 写入:
>      
>          
>          On 2021-08-30 11:24 p.m., Pan, Xinhui wrote:
>          > [AMD Official Use Only]
>          >
>          > [AMD Official Use Only]
>          >
>          > Unreserve root BO before return otherwise next allocation got deadlock.
>          >
>          > Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>          > ---
>          >   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++++------
>          >   1 file changed, 5 insertions(+), 6 deletions(-)
>          >
>          > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>          > index 85b292ed5c43..c9db7d2c5816 100644
>          > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>          > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>          > @@ -355,19 +355,18 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>          >                          DRM_DEBUG("Failed to allocate GEM object (%llu, %d, %llu, %d)\n",
>          >                                    size, initial_domain, args->in.alignment, r);
>          >                  }
>          > +
>          > +               if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
>          > +                       amdgpu_bo_unreserve(vm->root.bo);
>          >                  return r;
>          >          }
>          >
>          >          if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) {
>          > -               if (!r) {
>          > -                       struct amdgpu_bo *abo = gem_to_amdgpu_bo(gobj);
>          > +               struct amdgpu_bo *abo = gem_to_amdgpu_bo(gobj);
>          >
>          > -                       abo->parent = amdgpu_bo_ref(vm->root.bo);
>          > -               }
>          > +               abo->parent = amdgpu_bo_ref(vm->root.bo);
>          >                  amdgpu_bo_unreserve(vm->root.bo);
>          >          }
>          > -       if (r)
>          > -               return r;
>          
>          
>          The above early return seems to be needed for -ERESTARTSYS case.
>          
>          Andrey
>      
>      There are two returns. ERESTARTSYS and other error after retry are already handled by the first return in if {}. So the second return is not needed.
>      
>      Thanks
>      Xinhui
>
> Also we can do something like below which is simpler.

Yeah, I think that would be better. We could also change the "if (r) { 
if (r != -ERESTARTSYS) {" into just "if (r != -ERESTARTSYS)" then.

But good catch anyway.

Regards,
Christian.

> @@ -355,7 +355,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>                          DRM_DEBUG("Failed to allocate GEM object (%llu, %d, %llu, %d)\n",
>                                    size, initial_domain, args->in.alignment, r);
>                  }
> -               return r;
>          }
>   
>          if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) {
>   
>          
>          >
>          >          r = drm_gem_handle_create(filp, gobj, &handle);
>          >          /* drop reference from allocate - handle holds it now */
>          > --
>          > 2.25.1
>          
>      
>      
>