Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
>> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > That neatly explains the WARN. Not too happy about accumulating lots of
>> > backing storage specific processing into free_object, but that can be
>> > fixed up later (there is an obj->ops->release() pending).
>>
>> I'm more irked with the semantic overloading of object pinning. Might
>> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
>> of pinning them for their entire lifetime. But we can bikeshed that
>> later on ;-)
>
> Some merit to that argument, but it still feels correct to say that the
> stolen pages are pinned for their lifetime. Given obj->ops->release(),
> it does actually become simpler to not mess around with pin_count. So
> later it is.

I was more unhappy that pin_count has different meanings, until I've
noticed that we've fixed that up already with the introduction of
->pages_pin_count. Shouldn't stolen mem just hold a reference on that
one? After all unbinding from the gtt is ok with stolen memory, but
dropping the backing storage in the shrinker won't work. Not that we
currently use stolen for anything else than permanently pinned bos.

Reading through the code we have a bit an unclear relationship between
pages_pin_count and the (gtt) pin_count, so this approach would need
some clarification (especially around the shrinker).

And it would still leave us with a special case in free, so your
->release callback still has uses ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch