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

["Zhang, Yu" <yu.c.zhang@linux.intel.com>]

Hi Chris & Daniel,

   Thanks for your comments. Following are my understandings about the 
changes needed for this patch:

1> We do not need the guard page anymore between different VMs. For the 
very last physical GTT entry, let's keep it pointing to a guard page.

2> To reserve the GMs in our balloon code, drm_mm_reserve_node() is 
preferred than the current drm_mm_insert_node_in_range_generic().

   Am I correct? Please correct me if anything wrong. Thanks! :)

Yu


On 9/24/2014 9:21 PM, Chris Wilson wrote:
> 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?
>
> No. It is just that drm_mm_reserve_node() was added to have the precise
> semantics expected here with the strict checks. And should work better
> with dynamic ballooning...
> -Chris
>