Re: [PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> 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@intel.com>
> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > 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. Specifically omapdrm_priv->usergart looks like it
actually requires struct_mutex :-/
-Daniel
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel