* [PATCH] drm/amdgpu/userq: remove unnecessary NULL check
@ 2025-04-30 8:05 Dan Carpenter
2025-04-30 9:28 ` Sharma, Shashank
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Dan Carpenter @ 2025-04-30 8:05 UTC (permalink / raw)
To: Alex Deucher
Cc: Christian König, David Airlie, Simona Vetter,
Shashank Sharma, Sunil Khatri, Arvind Yadav,
Arunpravin Paneer Selvam, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
The "ticket" pointer points to in the middle of the &exec struct so it
can't be NULL. Remove the check.
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index b0e8098a3988..7505d920fb3d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
clear = false;
unlock = true;
/* The caller is already holding the reservation lock */
- } else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
+ } else if (dma_resv_locking_ctx(resv) == ticket) {
clear = false;
unlock = false;
/* Somebody else is using the BO right now */
--
2.47.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 8:05 [PATCH] drm/amdgpu/userq: remove unnecessary NULL check Dan Carpenter @ 2025-04-30 9:28 ` Sharma, Shashank 2025-04-30 9:49 ` Dan Carpenter 2025-04-30 12:35 ` Christian König 2025-04-30 12:28 ` Christian König 2025-04-30 14:20 ` Alex Deucher 2 siblings, 2 replies; 8+ messages in thread From: Sharma, Shashank @ 2025-04-30 9:28 UTC (permalink / raw) To: Dan Carpenter, Deucher, Alexander Cc: Koenig, Christian, David Airlie, Simona Vetter, Khatri, Sunil, Yadav, Arvind, Paneer Selvam, Arunpravin, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 1881 bytes --] [AMD Official Use Only - AMD Internal Distribution Only] Hello Dan, ________________________________ From: Dan Carpenter Sent: Wednesday, April 30, 2025 10:05 AM To: Deucher, Alexander Cc: Koenig, Christian; David Airlie; Simona Vetter; Sharma, Shashank; Khatri, Sunil; Yadav, Arvind; Paneer Selvam, Arunpravin; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org Subject: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check The "ticket" pointer points to in the middle of the &exec struct so it can't be NULL. Remove the check. Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c index b0e8098a3988..7505d920fb3d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) clear = false; unlock = true; /* The caller is already holding the reservation lock */ - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { + } else if (dma_resv_locking_ctx(resv) == ticket) { Its a Nack for me, There are a few situations (particularly during the first launch of the desktop, and also when eviction fence and new queue creation are working in parallel) where this ticket can be NULL, we observed it during the stress validation and hence added this check, Regards, Shashank clear = false; unlock = false; /* Somebody else is using the BO right now */ -- 2.47.2 [-- Attachment #2: Type: text/html, Size: 5206 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 9:28 ` Sharma, Shashank @ 2025-04-30 9:49 ` Dan Carpenter 2025-04-30 12:50 ` Sharma, Shashank 2025-04-30 12:35 ` Christian König 1 sibling, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2025-04-30 9:49 UTC (permalink / raw) To: Sharma, Shashank Cc: Deucher, Alexander, Koenig, Christian, David Airlie, Simona Vetter, Khatri, Sunil, Yadav, Arvind, Paneer Selvam, Arunpravin, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org On Wed, Apr 30, 2025 at 09:28:59AM +0000, Sharma, Shashank wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hello Dan, > > ________________________________ > From: Dan Carpenter > Sent: Wednesday, April 30, 2025 10:05 AM > To: Deucher, Alexander > Cc: Koenig, Christian; David Airlie; Simona Vetter; Sharma, Shashank; Khatri, Sunil; Yadav, Arvind; Paneer Selvam, Arunpravin; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org > Subject: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check > > The "ticket" pointer points to in the middle of the &exec struct so it > can't be NULL. Remove the check. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index b0e8098a3988..7505d920fb3d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) > clear = false; > unlock = true; > /* The caller is already holding the reservation lock */ > - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { > + } else if (dma_resv_locking_ctx(resv) == ticket) { > > Its a Nack for me, There are a few situations (particularly during the > first launch of the desktop, and also when eviction fence and new queue > creation are working in parallel) where this ticket can be NULL, we > observed it during the stress validation and hence added this check, > It shouldn't be NULL. It sounds like you are experiencing stack corruption and this is just a bandaid. drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c 566 static int 567 amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) 568 { 569 struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr); 570 struct amdgpu_vm *vm = &fpriv->vm; 571 struct amdgpu_device *adev = uq_mgr->adev; 572 struct amdgpu_bo_va *bo_va; 573 struct ww_acquire_ctx *ticket; 574 struct drm_exec exec; ^^^^^^^^^^^^^^^^^^^^^ The "exec" struct is declared on the stack. 575 struct amdgpu_bo *bo; 576 struct dma_resv *resv; 577 bool clear, unlock; 578 int ret = 0; 579 580 drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0); 581 drm_exec_until_all_locked(&exec) { 582 ret = amdgpu_vm_lock_pd(vm, &exec, 2); 583 drm_exec_retry_on_contention(&exec); 584 if (unlikely(ret)) { 585 DRM_ERROR("Failed to lock PD\n"); 586 goto unlock_all; 587 } 588 589 /* Lock the done list */ 590 list_for_each_entry(bo_va, &vm->done, base.vm_status) { 591 bo = bo_va->base.bo; 592 if (!bo) 593 continue; 594 595 ret = drm_exec_lock_obj(&exec, &bo->tbo.base); 596 drm_exec_retry_on_contention(&exec); 597 if (unlikely(ret)) 598 goto unlock_all; 599 } 600 } 601 602 spin_lock(&vm->status_lock); 603 while (!list_empty(&vm->moved)) { 604 bo_va = list_first_entry(&vm->moved, struct amdgpu_bo_va, 605 base.vm_status); 606 spin_unlock(&vm->status_lock); 607 608 /* Per VM BOs never need to bo cleared in the page tables */ 609 ret = amdgpu_vm_bo_update(adev, bo_va, false); 610 if (ret) 611 goto unlock_all; 612 spin_lock(&vm->status_lock); 613 } 614 615 ticket = &exec.ticket; ^^^^^^^^^^^^^^^^^^^^^ ticket is only set here. We know that &exec is non-NULL because it's declared on the stack. ticket is 4 bytes into the middle of a non-NULL struct. It is impossible for ticket to be NULL here. 616 while (!list_empty(&vm->invalidated)) { 617 bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va, 618 base.vm_status); 619 resv = bo_va->base.bo->tbo.base.resv; 620 spin_unlock(&vm->status_lock); 621 622 bo = bo_va->base.bo; 623 ret = amdgpu_userq_validate_vm_bo(NULL, bo); 624 if (ret) { 625 DRM_ERROR("Failed to validate BO\n"); 626 goto unlock_all; 627 } 628 629 /* Try to reserve the BO to avoid clearing its ptes */ 630 if (!adev->debug_vm && dma_resv_trylock(resv)) { 631 clear = false; 632 unlock = true; 633 /* The caller is already holding the reservation lock */ 634 } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { I've included the whole rest of the function so that we can see it is not set a second time. regards, dan carpenter 635 clear = false; 636 unlock = false; 637 /* Somebody else is using the BO right now */ 638 } else { 639 clear = true; 640 unlock = false; 641 } 642 643 ret = amdgpu_vm_bo_update(adev, bo_va, clear); 644 645 if (unlock) 646 dma_resv_unlock(resv); 647 if (ret) 648 goto unlock_all; 649 650 spin_lock(&vm->status_lock); 651 } 652 spin_unlock(&vm->status_lock); 653 654 ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, &exec); 655 if (ret) 656 DRM_ERROR("Failed to replace eviction fence\n"); 657 658 unlock_all: 659 drm_exec_fini(&exec); 660 return ret; 661 } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 9:49 ` Dan Carpenter @ 2025-04-30 12:50 ` Sharma, Shashank 0 siblings, 0 replies; 8+ messages in thread From: Sharma, Shashank @ 2025-04-30 12:50 UTC (permalink / raw) To: Dan Carpenter Cc: Deucher, Alexander, Koenig, Christian, David Airlie, Simona Vetter, Khatri, Sunil, Yadav, Arvind, Paneer Selvam, Arunpravin, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org On 30/04/2025 11:49, Dan Carpenter wrote: > On Wed, Apr 30, 2025 at 09:28:59AM +0000, Sharma, Shashank wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> Hello Dan, >> >> ________________________________ >> From: Dan Carpenter >> Sent: Wednesday, April 30, 2025 10:05 AM >> To: Deucher, Alexander >> Cc: Koenig, Christian; David Airlie; Simona Vetter; Sharma, Shashank; Khatri, Sunil; Yadav, Arvind; Paneer Selvam, Arunpravin; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org >> Subject: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check >> >> The "ticket" pointer points to in the middle of the &exec struct so it >> can't be NULL. Remove the check. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> index b0e8098a3988..7505d920fb3d 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) >> clear = false; >> unlock = true; >> /* The caller is already holding the reservation lock */ >> - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { >> + } else if (dma_resv_locking_ctx(resv) == ticket) { >> >> Its a Nack for me, There are a few situations (particularly during the >> first launch of the desktop, and also when eviction fence and new queue >> creation are working in parallel) where this ticket can be NULL, we >> observed it during the stress validation and hence added this check, >> > It shouldn't be NULL. It sounds like you are experiencing stack > corruption and this is just a bandaid. > > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > 566 static int > 567 amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) > 568 { > 569 struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr); > 570 struct amdgpu_vm *vm = &fpriv->vm; > 571 struct amdgpu_device *adev = uq_mgr->adev; > 572 struct amdgpu_bo_va *bo_va; > 573 struct ww_acquire_ctx *ticket; > 574 struct drm_exec exec; > ^^^^^^^^^^^^^^^^^^^^^ > The "exec" struct is declared on the stack. > > 575 struct amdgpu_bo *bo; > 576 struct dma_resv *resv; > 577 bool clear, unlock; > 578 int ret = 0; > 579 > 580 drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0); > 581 drm_exec_until_all_locked(&exec) { > 582 ret = amdgpu_vm_lock_pd(vm, &exec, 2); > 583 drm_exec_retry_on_contention(&exec); > 584 if (unlikely(ret)) { > 585 DRM_ERROR("Failed to lock PD\n"); > 586 goto unlock_all; > 587 } > 588 > 589 /* Lock the done list */ > 590 list_for_each_entry(bo_va, &vm->done, base.vm_status) { > 591 bo = bo_va->base.bo; > 592 if (!bo) > 593 continue; > 594 > 595 ret = drm_exec_lock_obj(&exec, &bo->tbo.base); > 596 drm_exec_retry_on_contention(&exec); > 597 if (unlikely(ret)) > 598 goto unlock_all; > 599 } > 600 } > 601 > 602 spin_lock(&vm->status_lock); > 603 while (!list_empty(&vm->moved)) { > 604 bo_va = list_first_entry(&vm->moved, struct amdgpu_bo_va, > 605 base.vm_status); > 606 spin_unlock(&vm->status_lock); > 607 > 608 /* Per VM BOs never need to bo cleared in the page tables */ > 609 ret = amdgpu_vm_bo_update(adev, bo_va, false); > 610 if (ret) > 611 goto unlock_all; > 612 spin_lock(&vm->status_lock); > 613 } > 614 > 615 ticket = &exec.ticket; > ^^^^^^^^^^^^^^^^^^^^^ > ticket is only set here. We know that &exec is non-NULL because it's > declared on the stack. ticket is 4 bytes into the middle of a non-NULL > struct. It is impossible for ticket to be NULL here. Yep, you are right. I just did a code review, and probably we added that NULL check before we had the right locks in place, and there was a race between eviction thread and the UQ create thread, causing corruption. Please feel free to use Acked-by: Shashank Sharma <shashank.sharma@amd.com> - Shashank > > 616 while (!list_empty(&vm->invalidated)) { > 617 bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va, > 618 base.vm_status); > 619 resv = bo_va->base.bo->tbo.base.resv; > 620 spin_unlock(&vm->status_lock); > 621 > 622 bo = bo_va->base.bo; > 623 ret = amdgpu_userq_validate_vm_bo(NULL, bo); > 624 if (ret) { > 625 DRM_ERROR("Failed to validate BO\n"); > 626 goto unlock_all; > 627 } > 628 > 629 /* Try to reserve the BO to avoid clearing its ptes */ > 630 if (!adev->debug_vm && dma_resv_trylock(resv)) { > 631 clear = false; > 632 unlock = true; > 633 /* The caller is already holding the reservation lock */ > 634 } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { > > I've included the whole rest of the function so that we can see it is not > set a second time. > > regards, > dan carpenter > > 635 clear = false; > 636 unlock = false; > 637 /* Somebody else is using the BO right now */ > 638 } else { > 639 clear = true; > 640 unlock = false; > 641 } > 642 > 643 ret = amdgpu_vm_bo_update(adev, bo_va, clear); > 644 > 645 if (unlock) > 646 dma_resv_unlock(resv); > 647 if (ret) > 648 goto unlock_all; > 649 > 650 spin_lock(&vm->status_lock); > 651 } > 652 spin_unlock(&vm->status_lock); > 653 > 654 ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, &exec); > 655 if (ret) > 656 DRM_ERROR("Failed to replace eviction fence\n"); > 657 > 658 unlock_all: > 659 drm_exec_fini(&exec); > 660 return ret; > 661 } > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 9:28 ` Sharma, Shashank 2025-04-30 9:49 ` Dan Carpenter @ 2025-04-30 12:35 ` Christian König 2025-04-30 12:52 ` Sharma, Shashank 1 sibling, 1 reply; 8+ messages in thread From: Christian König @ 2025-04-30 12:35 UTC (permalink / raw) To: Sharma, Shashank, Dan Carpenter, Deucher, Alexander Cc: David Airlie, Simona Vetter, Khatri, Sunil, Yadav, Arvind, Paneer Selvam, Arunpravin, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org On 4/30/25 11:28, Sharma, Shashank wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > > Hello Dan, > > -------------------------------------------------------------------------------- > *From:* Dan Carpenter > *Sent:* Wednesday, April 30, 2025 10:05 AM > *To:* Deucher, Alexander > *Cc:* Koenig, Christian; David Airlie; Simona Vetter; Sharma, Shashank; Khatri, > Sunil; Yadav, Arvind; Paneer Selvam, Arunpravin; amd-gfx@lists.freedesktop.org; > dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; kernel- > janitors@vger.kernel.org > *Subject:* [PATCH] drm/amdgpu/userq: remove unnecessary NULL check > > The "ticket" pointer points to in the middle of the &exec struct so it > can't be NULL. Remove the check. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/ > amdgpu/amdgpu_userq.c > index b0e8098a3988..7505d920fb3d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) > clear = false; > unlock = true; > /* The caller is already holding the reservation lock */ > - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { > + } else if (dma_resv_locking_ctx(resv) == ticket) { > > Its a Nack for me, There are a few situations (particularly during the first > launch of the desktop, and also when eviction fence and new queue creation are > working in parallel) where this ticket can be NULL, we observed it during the > stress validation and hence added this check, What that maybe before the code was moved around? As far as I can see the ticket can't be NULL any more. Regards, Christian. > > Regards, > Shashank > > > clear = false; > unlock = false; > /* Somebody else is using the BO right now */ > -- > 2.47.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 12:35 ` Christian König @ 2025-04-30 12:52 ` Sharma, Shashank 0 siblings, 0 replies; 8+ messages in thread From: Sharma, Shashank @ 2025-04-30 12:52 UTC (permalink / raw) To: Christian König, Dan Carpenter, Deucher, Alexander Cc: David Airlie, Simona Vetter, Khatri, Sunil, Yadav, Arvind, Paneer Selvam, Arunpravin, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org On 30/04/2025 14:35, Christian König wrote: > On 4/30/25 11:28, Sharma, Shashank wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> >> Hello Dan, >> >> -------------------------------------------------------------------------------- >> *From:* Dan Carpenter >> *Sent:* Wednesday, April 30, 2025 10:05 AM >> *To:* Deucher, Alexander >> *Cc:* Koenig, Christian; David Airlie; Simona Vetter; Sharma, Shashank; Khatri, >> Sunil; Yadav, Arvind; Paneer Selvam, Arunpravin; amd-gfx@lists.freedesktop.org; >> dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; kernel- >> janitors@vger.kernel.org >> *Subject:* [PATCH] drm/amdgpu/userq: remove unnecessary NULL check >> >> The "ticket" pointer points to in the middle of the &exec struct so it >> can't be NULL. Remove the check. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/ >> amdgpu/amdgpu_userq.c >> index b0e8098a3988..7505d920fb3d 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) >> clear = false; >> unlock = true; >> /* The caller is already holding the reservation lock */ >> - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { >> + } else if (dma_resv_locking_ctx(resv) == ticket) { >> >> Its a Nack for me, There are a few situations (particularly during the first >> launch of the desktop, and also when eviction fence and new queue creation are >> working in parallel) where this ticket can be NULL, we observed it during the >> stress validation and hence added this check, > What that maybe before the code was moved around? > > As far as I can see the ticket can't be NULL any more. Yes, that was before we sync'ed the locks between the two threads and moved the code. The NULL check was probably a leftover from the code carried forward. - Shashank > Regards, > Christian. > > >> Regards, >> Shashank >> >> >> clear = false; >> unlock = false; >> /* Somebody else is using the BO right now */ >> -- >> 2.47.2 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 8:05 [PATCH] drm/amdgpu/userq: remove unnecessary NULL check Dan Carpenter 2025-04-30 9:28 ` Sharma, Shashank @ 2025-04-30 12:28 ` Christian König 2025-04-30 14:20 ` Alex Deucher 2 siblings, 0 replies; 8+ messages in thread From: Christian König @ 2025-04-30 12:28 UTC (permalink / raw) To: Dan Carpenter, Alex Deucher Cc: David Airlie, Simona Vetter, Shashank Sharma, Sunil Khatri, Arvind Yadav, Arunpravin Paneer Selvam, amd-gfx, dri-devel, linux-kernel, kernel-janitors On 4/30/25 10:05, Dan Carpenter wrote: > The "ticket" pointer points to in the middle of the &exec struct so it > can't be NULL. Remove the check. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index b0e8098a3988..7505d920fb3d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) > clear = false; > unlock = true; > /* The caller is already holding the reservation lock */ > - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { > + } else if (dma_resv_locking_ctx(resv) == ticket) { > clear = false; > unlock = false; > /* Somebody else is using the BO right now */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amdgpu/userq: remove unnecessary NULL check 2025-04-30 8:05 [PATCH] drm/amdgpu/userq: remove unnecessary NULL check Dan Carpenter 2025-04-30 9:28 ` Sharma, Shashank 2025-04-30 12:28 ` Christian König @ 2025-04-30 14:20 ` Alex Deucher 2 siblings, 0 replies; 8+ messages in thread From: Alex Deucher @ 2025-04-30 14:20 UTC (permalink / raw) To: Dan Carpenter Cc: Alex Deucher, Christian König, David Airlie, Simona Vetter, Shashank Sharma, Sunil Khatri, Arvind Yadav, Arunpravin Paneer Selvam, amd-gfx, dri-devel, linux-kernel, kernel-janitors On Wed, Apr 30, 2025 at 4:13 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The "ticket" pointer points to in the middle of the &exec struct so it > can't be NULL. Remove the check. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Applied. Thanks! > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index b0e8098a3988..7505d920fb3d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -631,7 +631,7 @@ amdgpu_userq_validate_bos(struct amdgpu_userq_mgr *uq_mgr) > clear = false; > unlock = true; > /* The caller is already holding the reservation lock */ > - } else if (ticket && dma_resv_locking_ctx(resv) == ticket) { > + } else if (dma_resv_locking_ctx(resv) == ticket) { > clear = false; > unlock = false; > /* Somebody else is using the BO right now */ > -- > 2.47.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-05-01 13:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-30 8:05 [PATCH] drm/amdgpu/userq: remove unnecessary NULL check Dan Carpenter 2025-04-30 9:28 ` Sharma, Shashank 2025-04-30 9:49 ` Dan Carpenter 2025-04-30 12:50 ` Sharma, Shashank 2025-04-30 12:35 ` Christian König 2025-04-30 12:52 ` Sharma, Shashank 2025-04-30 12:28 ` Christian König 2025-04-30 14:20 ` Alex Deucher
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.