[Intel-gfx] [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Daniel Vetter]

On Wed, Nov 21, 2012 at 2:15 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, 21 Nov 2012 14:11:34 +0100, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Wed, Nov 21, 2012 at 01:04:03PM +0000, Chris Wilson wrote:
>> > As we may invoke the shrinker whilst trying to allocate memory to hold
>> > the gtt_space for this object, we need to be careful not to mark the
>> > drm_mm_node as activated (by assigning it to this object) before we
>> > have finished our sequence of allocations.
>> >
>> > Reported-by: Imre Deak <imre.deak at gmail.com>
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> > ---
>>
>> > @@ -3449,11 +3443,16 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>> >     }
>> >
>> >     if (obj->gtt_space == NULL) {
>> > +           struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>> > +
>> >             ret = i915_gem_object_bind_to_gtt(obj, alignment,
>> >                                               map_and_fenceable,
>> >                                               nonblocking);
>> >             if (ret)
>> >                     return ret;
>> > +
>> > +           if (!dev_priv->mm.aliasing_ppgtt)
>> > +                   i915_gem_gtt_bind_object(obj, obj->cache_level);
>>
>> Spurious hunk?
>
> Not really, I need to reorder the bind_object until after the assignment
> of obj->gtt_space and upon reflection it looked better if I did the bind
> there next to its compadre then amongst the assignments in the tail of
> bind_to_gtt(). Of course, this means that bind_to_gtt is now a grand
> misnomer.

Ok, I now see what's going on. Can't we do the bind together with the
mappable bind, i.e.

if (!has_global_mapping && (map_and_fenceable || !aliasing_ppgtt))
        i915_gem_gtt_bind_object()

Maybe also mention that the gtt binding needs to be moved around, for
dummies like me. Wrt renaming i915_gem_object_bind_to_gtt, I don't
have a good idea to denote the "reserve some space in the abstract
address space and update respective lrus" action it's doing. I guess
we can ponder that again with real ppgtt support, where binding into
the address space and binding into pagetables are different steps
indeed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch