Re: [PATCH v3 3/4] drm/ttm: convert to unified vma offset manager

From: Daniel Vetter <daniel@ffwll.ch>
To: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Martin Peres <martin.peres@labri.fr>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Ben Skeggs <bskeggs@redhat.com>,
	Alex Deucher <alexander.deucher@amd.com>,
	Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH v3 3/4] drm/ttm: convert to unified vma offset manager
Date: Thu, 18 Jul 2013 13:02:40 +0200	[thread overview]
Message-ID: <CAKMK7uG6YT2peaCvMP5iLhFGKrb-mHRTHAQsdEDNic9cmsTsdg@mail.gmail.com> (raw)
In-Reply-To: <51E7AD18.4050100@vmware.com>

On Thu, Jul 18, 2013 at 10:53 AM, Thomas Hellstrom
<thellstrom@vmware.com> 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