[Intel-gfx] [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Zhang, Yu]

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?

>
> 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!


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?

> -Daniel
>

Thanks
Yu