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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel