Hi Daniel, On Tuesday, 3 April 2018 12:07:31 EEST Daniel Vetter wrote: > On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote: > > On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote: > >> On 28/03/18 14:41, Daniel Vetter wrote: > >>> The only thing that omap_gem_free_object does that might need the > >>> magic protection of struct_mutex (of keeping all objects alive if that > >>> lock is held, even if the last reference is gone) is the mm_list > >>> manipulation. > >>> > >>> But that is already protected by the separate omapdrm->list_lock, > >>> which means struct_mutex is an entirely internal lock for omapdrm. > >>> Everything else is just releasing resources, which is all protected > >>> already by the various subsystems and allocators. > >>> > >>> To make this even more obvious we could do an > >>> s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But > >>> since omapdrm is a lot bigger and a lot more active I'll refrain from > >>> that - this is better done by omapdrm developers at some suitable time > >>> in the future. > >>> > >>> v2: Just auditing the code isn't enough, I actually have to remove > >>> the now wrong locking check in omap_gem_free_object ... > >>> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > >>> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > >> > >> This version works fine. I'll pick this to omapdrm branch. Thanks! > > > > Unless I'm mistaken (this is only based on code analysis), a WARN_ON could > > also be triggered through the following call stack. > > > > gem_free_object_unlocked() > > omap_gem_free_object() > > evict() > > evict_entry() > > mmap_offset() > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)) > > > > There could be other such call stacks. > > > > I don't think we should switch to gem_free_object_unlocked() until all > > usage of struct_mutex is removed from the omapdrm driver. > > Hm, indeed I missed that one. I'm not entirely sure how the tiler locking > will pan out though, and whether there's not some stuff that won't be > protected anymore. I reviewed locking through the driver when writing "drm/omap: gem: Replace struct_mutex usage with omap_obj private lock", but I'm also concerned that I might have missed something and introduced a race condition :-/ > Specifically omapdrm_priv->usergart looks like it actually requires > struct_mutex :-/ I think you're right. Or at rather not struct_mutex, but an omapdrm-specific gart mutex. I'm not too familiar with that code, I suppose I'll have to dive in. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
