From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Yu" Subject: Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic Date: Wed, 24 Sep 2014 20:35:50 +0800 Message-ID: <5422BAA6.8090105@linux.intel.com> References: <1411152428-7226-1-git-send-email-jike.song@intel.com> <1411152428-7226-3-git-send-email-jike.song@intel.com> <20140919080557.GC21738@nuc-i3427.alporthouse.com> <20140919200000.GH21738@nuc-i3427.alporthouse.com> <20140923082626.GV15734@phenom.ffwll.local> <20140923091902.GG8727@nuc-i3427.alporthouse.com> <20140923112503.GD15734@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 090DA6E433 for ; Wed, 24 Sep 2014 05:41:58 -0700 (PDT) In-Reply-To: <20140923112503.GD15734@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , Chris Wilson , "Tian, Kevin" , "Song, Jike" , "Vetter, Daniel" , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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