search for: ww_mutex

...>> to the driver is ok, as long as they don't conflict with anything possible >> used by their own fence implementation. > Well that's almost what we have right now with the exception that > drivers are allowed (actually must for correctness when updating > fences) the ww_mutexes for dma-bufs (or other buffer objects). In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()? &...

...driver is ok, as long as they don't conflict with anything possible >>> used by their own fence implementation. >> Well that's almost what we have right now with the exception that >> drivers are allowed (actually must for correctness when updating >> fences) the ww_mutexes for dma-bufs (or other buffer objects). > > In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. > > But how does that then work right now? My impression was that it's mandatory for drivers to call fenc...

...as they don't conflict with anything >>> possible >>> used by their own fence implementation. >> >> Well that's almost what we have right now with the exception that >> drivers are allowed (actually must for correctness when updating >> fences) the ww_mutexes for dma-bufs (or other buffer objects). > > > In this case sorry for so much noise. I really haven't looked in so much > detail into anything but Maarten's Radeon patches. > > But how does that then work right now? My impression was that it's mandatory > for drive...

...n't conflict with anything >>>> possible >>>> used by their own fence implementation. >>> Well that's almost what we have right now with the exception that >>> drivers are allowed (actually must for correctness when updating >>> fences) the ww_mutexes for dma-bufs (or other buffer objects). >> >> In this case sorry for so much noise. I really haven't looked in so much >> detail into anything but Maarten's Radeon patches. >> >> But how does that then work right now? My impression was that it's mandatory...

Am 22.07.2014 17:17, schrieb Daniel Vetter: > On Tue, Jul 22, 2014 at 3:45 PM, Christian K?nig > <deathsimple at vodafone.de> wrote: >>> Would that be something you can agree to? >> >> No, the whole enable_signaling stuff should go away. No callback from the >> driver into the fence code, only the other way around. >> >> fence->signaled as well

...cking primitives local > to the driver is ok, as long as they don't conflict with anything possible > used by their own fence implementation. Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects). Locking correctness is enforced with some extremely nasty lockdep annotations + additional debugging infrastructure enabled with CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold dma-buf ww_mutexes while updating fences or waiting for them....

...a negative error code otherwise. > + */ > +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo, > + unsigned long pl_flag) > +{ > + int i, ret; > + struct ttm_operation_ctx ctx = { false, false }; I think would be good to have a lockdep_assert_held here for the ww_mutex. Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations we call the structure tracking the fences+lock the "reservation", but the naming scheme used is _lock/_unlock. I think would be good to be consistent with that, and use _locked here. Especially for a very simpli...

...a negative error code otherwise. > + */ > +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo, > + unsigned long pl_flag) > +{ > + int i, ret; > + struct ttm_operation_ctx ctx = { false, false }; I think would be good to have a lockdep_assert_held here for the ww_mutex. Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations we call the structure tracking the fences+lock the "reservation", but the naming scheme used is _lock/_unlock. I think would be good to be consistent with that, and use _locked here. Especially for a very simpli...

On Tue, Jul 22, 2014 at 3:45 PM, Christian K?nig <deathsimple at vodafone.de> wrote: >> Would that be something you can agree to? > > > No, the whole enable_signaling stuff should go away. No callback from the > driver into the fence code, only the other way around. > > fence->signaled as well as fence->wait should become mandatory and only > called from

...s long as they don't conflict with anything possible >>>> used by their own fence implementation. >>> Well that's almost what we have right now with the exception that >>> drivers are allowed (actually must for correctness when updating >>> fences) the ww_mutexes for dma-bufs (or other buffer objects). >> In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. >> >> But how does that then work right now? My impression was that it's mandatory for drivers to ca...

Hi, > I think would be good to have a lockdep_assert_held here for the ww_mutex. > > Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations > we call the structure tracking the fences+lock the "reservation", but the > naming scheme used is _lock/_unlock. > > I think would be good to be consistent with that, and use _locked here...

Am 27.08.23 um 19:54 schrieb Dmitry Osipenko: > Add simple kref_put_dma_resv() helper that wraps around kref_put_ww_mutex() > for drivers that needs to lock dma-resv on kref_put(). > > It's not possible to easily add this helper to kref.h because of the > headers inclusion dependency, hence add it to dma-resv.h. I was never really a big fan of kref_put_mutex() in the first place. The main advantage c...

...ave to agree with Danilo here. Especially you have cases where you usually lock BO->VM (for example eviction) as well as cases where you need to lock VM->BO (command submission). Because of this in amdgpu we used (or abused?) the dma_resv of the root BO as lock for the VM. Since this is a ww_mutex locking it in both VM, BO as well as BO, VM order works. Regards, Christian. > , for example a vm and bo resv when putting the vm_bo, we need to keep > additional strong references for the bo / vm pointer we use for > unlocking. Hence putting the vm_bo under those locks can never lead...

...m_vram_pin_reserved(struct drm_gem_vram_object *gbo, >>> + unsigned long pl_flag) >>> +{ >>> + int i, ret; >>> + struct ttm_operation_ctx ctx = { false, false }; >> I think would be good to have a lockdep_assert_held here for the ww_mutex. >> >> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations >> we call the structure tracking the fences+lock the "reservation", but the >> naming scheme used is _lock/_unlock. >> >> I think would be good to be consistent with that...

...on't conflict with anything possible >>>>> used by their own fence implementation. >>>> Well that's almost what we have right now with the exception that >>>> drivers are allowed (actually must for correctness when updating >>>> fences) the ww_mutexes for dma-bufs (or other buffer objects). >>> In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. >>> >>> But how does that then work right now? My impression was that it's mandatory for d...

...here. Especially you have cases where you > usually lock BO->VM (for example eviction) as well as cases where you > need to lock VM->BO (command submission). > > Because of this in amdgpu we used (or abused?) the dma_resv of the > root BO as lock for the VM. Since this is a ww_mutex locking it in > both VM, BO as well as BO, VM order works. Yes, gpuvm is doing the same. (although not necessarily using the page-table root bo, but any bo of the driver's choice). But I read it as Danilo feared the case where the VM destructor was called with a VM resv (or possibly bo...

...gt; + */ > > +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo, > > + unsigned long pl_flag) > > +{ > > + int i, ret; > > + struct ttm_operation_ctx ctx = { false, false }; > > I think would be good to have a lockdep_assert_held here for the ww_mutex. > > Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations > we call the structure tracking the fences+lock the "reservation", but the > naming scheme used is _lock/_unlock. > > I think would be good to be consistent with that, and use _locked here...

...t with anything possible >>>>>> used by their own fence implementation. >>>>> Well that's almost what we have right now with the exception that >>>>> drivers are allowed (actually must for correctness when updating >>>>> fences) the ww_mutexes for dma-bufs (or other buffer objects). >>>> In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. >>>> >>>> But how does that then work right now? My impression was that it's man...

A kernel test bot reported a problem with the locktorture testcase that was triggered by the GEM VRAM helpers. ... [ 10.004734] RIP: 0010:ttm_bo_validate+0x41/0x141 [ttm] ... [ 10.015669] ? kvm_sched_clock_read+0x5/0xd [ 10.016157] ? get_lock_stats+0x11/0x3f [ 10.016607] drm_gem_vram_pin+0x77/0xa2 [drm_vram_helper] [ 10.017229]

A kernel test bot reported a problem with the locktorture testcase that was triggered by the GEM VRAM helpers. ... [ 10.004734] RIP: 0010:ttm_bo_validate+0x41/0x141 [ttm] ... [ 10.015669] ? kvm_sched_clock_read+0x5/0xd [ 10.016157] ? get_lock_stats+0x11/0x3f [ 10.016607] drm_gem_vram_pin+0x77/0xa2 [drm_vram_helper] [ 10.017229]