From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 09520C433B4 for ; Wed, 12 May 2021 13:02:13 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 60DC261287 for ; Wed, 12 May 2021 13:02:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 60DC261287 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 557876EB98; Wed, 12 May 2021 13:02:11 +0000 (UTC) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id A77D36EB98; Wed, 12 May 2021 13:02:09 +0000 (UTC) IronPort-SDR: mz37/KCvRAK/+6Bwc5enMohDDAmTitMRX2JhERlCtotZPLms+Uog84RYMXc32VGOdFeIUHvlTG P1DYajJlPlxA== X-IronPort-AV: E=McAfee;i="6200,9189,9981"; a="179963349" X-IronPort-AV: E=Sophos;i="5.82,293,1613462400"; d="scan'208";a="179963349" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 May 2021 06:02:06 -0700 IronPort-SDR: cfTayY3noWvjLgRlrcBfD+8S9sVW2vG5X0AHxR/ciDX6w4SCFEv/bw6BHISIOpnHtWhzHQ4LtB g91OWBZt57/A== X-IronPort-AV: E=Sophos;i="5.82,293,1613462400"; d="scan'208";a="609921226" Received: from kjeldbeg-mobl2.ger.corp.intel.com ([10.249.254.168]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 May 2021 06:02:05 -0700 Message-ID: <6e317ee2b22546a2333d3098f5cda1a59da62a1a.camel@linux.intel.com> Subject: Re: [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend From: Thomas =?ISO-8859-1?Q?Hellstr=F6m?= To: Christian =?ISO-8859-1?Q?K=F6nig?= , Thomas =?ISO-8859-1?Q?Hellstr=F6m?= "(Intel)" , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Date: Wed, 12 May 2021 15:02:02 +0200 In-Reply-To: References: <20210511132525.377190-1-thomas.hellstrom@linux.intel.com> <20210511132525.377190-7-thomas.hellstrom@linux.intel.com> <8ac6bc5c-17c0-2ffd-7f8c-823ab3c8a858@amd.com> <88ea8e22-3314-60a4-8f4b-0b37de444b1d@shipmail.org> <8551810c-a095-3906-d982-7bc409140c48@amd.com> <0b7f32d8-bfb9-84dd-fea7-556dddded1cc@linux.intel.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote: > Am 12.05.21 um 09:05 schrieb Thomas Hellström: > > On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote: > > > Am 11.05.21 um 16:28 schrieb Thomas Hellström: > > > > On 5/11/21 4:09 PM, Christian König wrote: > > > > > > > > > > Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel): > > > > > > On 5/11/21 3:58 PM, Christian König wrote: > > > > > > > Am 11.05.21 um 15:25 schrieb Thomas Hellström: > > > > > > > > Most logical place to introduce TTM buffer objects is > > > > > > > > as an > > > > > > > > i915 > > > > > > > > gem object backend. We need to add some ops to account > > > > > > > > for > > > > > > > > added > > > > > > > > functionality like delayed delete and LRU list > > > > > > > > manipulation. > > > > > > > > > > > > > > > > Initially we support only LMEM and SYSTEM memory, but > > > > > > > > SYSTEM > > > > > > > > (which in this case means evicted LMEM objects) is not > > > > > > > > visible to i915 GEM yet. The plan is to move the i915 > > > > > > > > gem > > > > > > > > system > > > > > > > > region > > > > > > > > over to the TTM system memory type in upcoming patches. > > > > > > > > > > > > > > > > We set up GPU bindings directly both from LMEM and from > > > > > > > > the > > > > > > > > system > > > > > > > > region, > > > > > > > > as there is no need to use the legacy TTM_TT memory > > > > > > > > type. > > > > > > > > We reserve > > > > > > > > that for future porting of GGTT bindings to TTM. > > > > > > > > > > > > > > > > There are some changes to TTM to allow for purging > > > > > > > > system > > > > > > > > memory > > > > > > > > buffer > > > > > > > > objects and to refuse swapping of some objects: > > > > > > > > Unfortunately i915 > > > > > > > > gem > > > > > > > > still relies heavily on short-term object pinning, and > > > > > > > > we've > > > > > > > > chosen to > > > > > > > > keep short-term-pinned buffer objects on the TTM LRU > > > > > > > > lists > > > > > > > > for now, > > > > > > > > meaning that we need some sort of mechanism to tell TTM > > > > > > > > they are not > > > > > > > > swappable. A longer term goal is to get rid of the > > > > > > > > short- > > > > > > > > term > > > > > > > > pinning. > > > > > > > Well just use the eviction_valuable interface for this. > > > > > > Yes, we do that for vram/lmem eviction, but we have nothing > > > > > > similar > > > > > > for system swapping. Do I understand you correctly that you > > > > > > want me > > > > > > to add a call to eviction_valuable() also for that instead > > > > > > of > > > > > > swap_possible()? > > > > > You should already have that. eviction_valuable is called in > > > > > both > > > > > cases. > > > > > > > > > Hmm. I can only see it called from ttm_mem_evict_first() which > > > > is > > > > not > > > > in the swapping path? Or do I miss something? > > > Mhm, looks like my recollection was wrong. We should probably > > > move > > > the > > > call into the ttm_bo_evict_swapout_allowable() function. > > Yes, I think we also need a convention whether it's called dma_resv > > locked or not, since the helper accesses bo->mem, which should > > really > > only be done under reservation. At the same point, there is value > > in > > calling this function while holding the LRU lock. > > You actually need to call it while holding the lock because eviction > otherwise ends up in an endless loop. > > Trying to fix that for years, but so far no luck with that. > > > Also, I wonder whether implementations of this callback might > > encounter > > unexpected data when called from the swapout path, because at least > > the > > helper assumes it not in system memory, since it is accessing bo- > > > mem.start. > > So unless we use a separate callback for swapout, there's some > > auditing > > to be done. > > Please audit the existing callbacks and move the callback into the > function after doing that. > > Thanks, > Christian. Would it be OK if I also move the kref_get_unless_zero() to before ttm_bo_evict_swapout_allowable() to make the code less sensitive to surprises? /Thomas > > > > > Pls let me know what you think. > > Thanks, > > Thomas > > > > > > > > > Christian. > > > > > > > Thanks, > > > > > > > > Thomas > > > > > > > > > > > > > > >