Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 24, 2014 at 08:35:50PM +0800, Zhang, Yu wrote:
> Hi Daniel & Chris,
> 
>   Thank you very much for your comments, And sorry for my late reply.:) I
> was focusing on other tasks previously.
>   See my questions below:
> 
> On 9/23/2014 7:25 PM, Daniel Vetter wrote:
> >On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
> >>On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
> >>>On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> >>>>On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> >>>>>>From: Chris Wilson
> >>>>>>The implementation also looks backwards. To work correctly with the GTT
> >>>>>>allocator, you need to preallocate the reserved space such that it can
> >>>>>>only allocate from the allowed ranges. Similarly, it should evict any
> >>>>>>conflicting nodes when deballooning.
> >>>>>
> >>>>>Could you elaborate a bit for above suggestion?
> >>>>
> >>>>My expectation was that the dev_priv->gtt.base.vm would contain exactly
> >>>>two holes after setup (in the mappable and non-mappable range). To do
> >>>>that you would explicitly reserve everything barred from this client
> >>>>using a set of drm_mm_reserve_node()
> >>>
> >>>Essentially a reserve_node implements what you open-code with
> >>>insert_node_range right now.
> >>
> >>Heh, there is a big difference. One inserts exactly where you ask and
> >>fails if it conflicts, the other inserts where it feels like within that
> >>range.
> 
> Do you mean drm_mm_search_free_in_range_generic() may not get reserve the
> exact range we are expecting to? Is this why you'd prefer the
> drm_mm_reserve_node()?
> 
> Besides, the ggtt_vm->mm is just initialized right before the ballooning
> code in routine i915_gem_setup_global_gtt(), so is there any chance the
> range to be partitioned out is already reserved by someone else?

Not really, but simply using drm_mm_reserve_node makes the intent much
clearer. Also if we go around to runtime ballooning failing hard is
probably what we want anyway.

> >Well if the the requested size matches the range exactly then it will be
> >the same. Which iirc is what's going on here I think.
> >
> >>>One issue aside with both this and with the PDE reservations for gen7 is
> >>>that there are now other thins in the ggtt drm_mm allocator than just gem
> >>>objects. Which means our debugfs files are now less useful.
> >>>
> >>>It might be useful to augment that dumper with one that dumps everything.
> >>>We could add a few bits of driver-private tags in drm_mm_node (there's
> >>>space) to figure out what kind of object it is. Would be a great follow-up
> >>>task.
> >>
> >>I think moving the other way and making them all objects so that we can
> >>tie them into evection and the shrinker, use more interesting allocation
> >>strategies, improve integration with debugging etc.
> >
> >Hm, not sure yet since it will be a lot of work at least. But I guess we
> >could untangle the meaning of obj->pin a bit and add an unbind vfunc which
> >adds some magic. But there's a lot of stuff attached to a gem bo that just
> >doesn't make a lot of sense really, so maybe a better option would be to
> >subclass a struct i915_ggtt_vma with special magic. Dunno really.
> 
> Sorry, not sure what these comments are about. :) I'll need time to read the
> code. Could you please elaborate a bit? Thanks!

We're discussing different options to unify the objects allocated from the
ggtt drm_mm. Not terribly relevant for your work directly.

> P.S. about the guard page: for now, the current logic reserves a guard page
> between different guests and at the very last entry of the whole physical
> GTT. the previous comments says: "The CS prefetcher happens everywhere and
> so can read from the end of one range into the beginning of another
> clients". So I guess the guard page in current patch is necessary, right?

tbh I'm not really sure whether we even need the guard page at the very
end of the gtt any more on recent platforms. And we definitely don't need
any guard page between different allocations on hsw, so I think we can
just remove that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch