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

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

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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel