On Thu, Jul 18, 2013 at 10:53 AM, Thomas Hellstrom <thellstrom@xxxxxxxxxx> wrote: > A quick look, but not a full review: > > Looks mostly good, but it looks like the TTM vm lock isn't needed at all > anymore (provided the vma offset manager is properly protected), since > kref_get_unless_zero() is used when a reference after lookup is taken. > (please see the kref_get_unless_zero documentation examples). When > drm_vma_offset_remove() is called, the kref value is unconditionally zero, > and that should block any successful lookup. > > Otherwise, if the vma offset manager always needs external locking to make > lookup + referencing atomic, then perhaps locking should be completely > left to the caller? Somehow I've thought plain gem drivers had this fixed, but looks like I've never gotten around to it. So we need to rework things anyway. What about a drm_vma_offset_lookup_unlocked which just checks that the caller is holding the offset manager's lock? That way everyone can follow up with the get_unless_zero dance correctly. And ttm could drop it's own vm lock. I've considered whether we should just move the vma node into struct drm_gem_object and let the offset manager do the dance, but that's much more invasive. And I'm not sure it's the right thing to do yet. So I think we should consider this only as a follow-up series. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
