* [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails @ 2021-08-31 3:24 Pan, Xinhui 2021-08-31 4:03 ` Andrey Grodzovsky 0 siblings, 1 reply; 5+ messages in thread From: Pan, Xinhui @ 2021-08-31 3:24 UTC (permalink / raw) To: amd-gfx@lists.freedesktop.org; +Cc: Koenig, Christian, Deucher, Alexander [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; r = drm_gem_handle_create(filp, gobj, &handle); /* drop reference from allocate - handle holds it now */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails 2021-08-31 3:24 [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails Pan, Xinhui @ 2021-08-31 4:03 ` Andrey Grodzovsky 2021-08-31 5:38 ` Pan, Xinhui 0 siblings, 1 reply; 5+ messages in thread From: Andrey Grodzovsky @ 2021-08-31 4:03 UTC (permalink / raw) To: Pan, Xinhui, amd-gfx@lists.freedesktop.org Cc: Koenig, Christian, Deucher, Alexander 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 > > r = drm_gem_handle_create(filp, gobj, &handle); > /* drop reference from allocate - handle holds it now */ > -- > 2.25.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails 2021-08-31 4:03 ` Andrey Grodzovsky @ 2021-08-31 5:38 ` Pan, Xinhui 2021-08-31 5:47 ` Pan, Xinhui 0 siblings, 1 reply; 5+ messages in thread From: Pan, Xinhui @ 2021-08-31 5:38 UTC (permalink / raw) To: Grodzovsky, Andrey, amd-gfx@lists.freedesktop.org Cc: Koenig, Christian, Deucher, Alexander 在 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 > > r = drm_gem_handle_create(filp, gobj, &handle); > /* drop reference from allocate - handle holds it now */ > -- > 2.25.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails 2021-08-31 5:38 ` Pan, Xinhui @ 2021-08-31 5:47 ` Pan, Xinhui 2021-08-31 6:19 ` Christian König 0 siblings, 1 reply; 5+ messages in thread From: Pan, Xinhui @ 2021-08-31 5:47 UTC (permalink / raw) To: Grodzovsky, Andrey, amd-gfx@lists.freedesktop.org Cc: Koenig, Christian, Deucher, Alexander 在 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. @@ -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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails 2021-08-31 5:47 ` Pan, Xinhui @ 2021-08-31 6:19 ` Christian König 0 siblings, 0 replies; 5+ messages in thread From: Christian König @ 2021-08-31 6:19 UTC (permalink / raw) To: Pan, Xinhui, Grodzovsky, Andrey, amd-gfx@lists.freedesktop.org Cc: Koenig, Christian, Deucher, Alexander 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 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-31 6:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-08-31 3:24 [PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails Pan, Xinhui 2021-08-31 4:03 ` Andrey Grodzovsky 2021-08-31 5:38 ` Pan, Xinhui 2021-08-31 5:47 ` Pan, Xinhui 2021-08-31 6:19 ` Christian König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox