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=-14.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 044D0C48BE0 for ; Fri, 11 Jun 2021 11:30:44 +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 B6952613C8 for ; Fri, 11 Jun 2021 11:30:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B6952613C8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.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 3193D6EE6F; Fri, 11 Jun 2021 11:30:43 +0000 (UTC) Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by gabe.freedesktop.org (Postfix) with ESMTPS id 38E806EE6F for ; Fri, 11 Jun 2021 11:30:42 +0000 (UTC) Received: by mail-wm1-x332.google.com with SMTP id l7-20020a05600c1d07b02901b0e2ebd6deso8307837wms.1 for ; Fri, 11 Jun 2021 04:30:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding:content-language; bh=RoC9doV+7XnrhBdmrOggcmPU+MqSliIk6SEGm0l7wIk=; b=rxas9qgXzKPrwLyyGxAEFpFempNCO/9eXOsGa7j3MHLvPdaBc+BFD+tP7qNvHv0OC9 6NZKL9KI5sHuVziainfOhGhBOl+xXVME0P/NdrDpNbMrerwVWW1tailbHo+vshDTWUH4 kv9iDubukhvGUT0B5/LXtyHZ0WCOTbUsF+eE+6Lh/XlWKUwuOEc0nbHM4s0LCcF4LfHE 26yXv3IsVVOPFKHKFMjxRf1NGHyumSPoxszsnujb/utrjauYJuB87FE95YanF08R1jMn rXoYTaayJ9ijaW3Puo6OsdKd5GFota8EodqPIao9lWtCmhwYKy0gDDAasmrKVB1sK2Wd SgZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=RoC9doV+7XnrhBdmrOggcmPU+MqSliIk6SEGm0l7wIk=; b=AVRtvp6IZUyE0bBheO5EOxS/WbGKNYWrBWWetMBWu1HJVNW5EfezBBjBsH/zrN11Gr r5jIKpXgwGobIUPqk3e9Xx/EmTwIGk+ng7BGxOz1iJc1oGLmFEMBRTHgaEQNvFVfxfKc fdKVw2eGpnoxDqdLW/Y/26joxJsVculz5XJf8f2hCcDSzbqhUoQzbVmhAXboI+ocDxAC 58SwHaxBoAu4lQrXBi15Q1eycG+0rXr5DpNYRlzl55HL4Ud3BnBuFVEb1WJ4qm4gz0D+ ObULPqjdRZScgo7mYOjD/jcKoGp2QamrAK7QldsvqFr8Vj9VscZ+dVzC1ASJU57bnv1B i3AQ== X-Gm-Message-State: AOAM531XeJWqQO7ylFb122o2lnW4A2Cgtwg9zC+X0W9WFOUbVu02g+et MvTgp1nLSEuCYFcjCs0iSKk0UlB9A1Q= X-Google-Smtp-Source: ABdhPJx3M14SWxA8OON+6K2pMVFd+U8X8d8ZTpBKmxMnSXnD3t4JFG1J1Ztvf4y+44EcnGInT2LJgg== X-Received: by 2002:a05:600c:243:: with SMTP id 3mr3491793wmj.35.1623411040789; Fri, 11 Jun 2021 04:30:40 -0700 (PDT) Received: from ?IPv6:2a02:908:1252:fb60:bd94:4b9a:99c4:4bc1? ([2a02:908:1252:fb60:bd94:4b9a:99c4:4bc1]) by smtp.gmail.com with ESMTPSA id c2sm12858749wmf.24.2021.06.11.04.30.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Jun 2021 04:30:40 -0700 (PDT) Subject: Re: [PATCH 1/4] drm/ttm: add a pointer to the allocating BO into ttm_resource To: =?UTF-8?Q?Thomas_Hellstr=c3=b6m_=28Intel=29?= , matthew.auld@intel.com, dri-devel@lists.freedesktop.org References: <20210610110559.1758-1-christian.koenig@amd.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: Date: Fri, 11 Jun 2021 13:30:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US 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" Am 11.06.21 um 07:34 schrieb Thomas Hellström (Intel): > Hi, Christian, > > I know you have a lot on your plate, and that the drm community is a > bit lax about following the kernel patch submitting guidelines, but > now that we're also spinning up a number of Intel developers on TTM > could we please make a better effort with cover letters and commit > messages so that they understand what the purpose and end goal of the > series is. A reviewer shouldn't have to look at the last patch to try > to get an understanding what the series is doing and why. Sorry, that was send out this early unintentionally. See it more like an RFC. > > On 6/10/21 1:05 PM, Christian König wrote: >> We are going to need this for the next patch > > >> and it allows us to clean >> up amdgpu as well. > > The amdgpu changes are not reflected in the commit title. > > >> >> Signed-off-by: Christian König >> --- >>   drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 47 ++++++++------------- >>   drivers/gpu/drm/ttm/ttm_resource.c          |  1 + >>   include/drm/ttm/ttm_resource.h              |  1 + >>   3 files changed, 19 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >> index 194f9eecf89c..8e3f5da44e4f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >> @@ -26,23 +26,12 @@ >>     #include "amdgpu.h" >>   -struct amdgpu_gtt_node { >> -    struct ttm_buffer_object *tbo; >> -    struct ttm_range_mgr_node base; >> -}; >> - >>   static inline struct amdgpu_gtt_mgr * >>   to_gtt_mgr(struct ttm_resource_manager *man) >>   { >>       return container_of(man, struct amdgpu_gtt_mgr, manager); >>   } >>   -static inline struct amdgpu_gtt_node * >> -to_amdgpu_gtt_node(struct ttm_resource *res) >> -{ >> -    return container_of(res, struct amdgpu_gtt_node, base.base); >> -} >> - >>   /** >>    * DOC: mem_info_gtt_total >>    * >> @@ -107,9 +96,9 @@ const struct attribute_group >> amdgpu_gtt_mgr_attr_group = { >>    */ >>   bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *res) >>   { >> -    struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(res); >> +    struct ttm_range_mgr_node *node = to_ttm_range_mgr_node(res); >>   -    return drm_mm_node_allocated(&node->base.mm_nodes[0]); >> +    return drm_mm_node_allocated(&node->mm_nodes[0]); >>   } >>     /** >> @@ -129,7 +118,7 @@ static int amdgpu_gtt_mgr_new(struct >> ttm_resource_manager *man, >>   { >>       struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man); >>       uint32_t num_pages = PFN_UP(tbo->base.size); >> -    struct amdgpu_gtt_node *node; >> +    struct ttm_range_mgr_node *node; >>       int r; >>         spin_lock(&mgr->lock); >> @@ -141,19 +130,17 @@ static int amdgpu_gtt_mgr_new(struct >> ttm_resource_manager *man, >>       atomic64_sub(num_pages, &mgr->available); >>       spin_unlock(&mgr->lock); >>   -    node = kzalloc(struct_size(node, base.mm_nodes, 1), GFP_KERNEL); >> +    node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL); >>       if (!node) { >>           r = -ENOMEM; >>           goto err_out; >>       } >>   -    node->tbo = tbo; >> -    ttm_resource_init(tbo, place, &node->base.base); >> - >> +    ttm_resource_init(tbo, place, &node->base); >>       if (place->lpfn) { >>           spin_lock(&mgr->lock); >>           r = drm_mm_insert_node_in_range(&mgr->mm, >> -                        &node->base.mm_nodes[0], >> +                        &node->mm_nodes[0], >>                           num_pages, tbo->page_alignment, >>                           0, place->fpfn, place->lpfn, >>                           DRM_MM_INSERT_BEST); >> @@ -161,14 +148,14 @@ static int amdgpu_gtt_mgr_new(struct >> ttm_resource_manager *man, >>           if (unlikely(r)) >>               goto err_free; >>   -        node->base.base.start = node->base.mm_nodes[0].start; >> +        node->base.start = node->mm_nodes[0].start; >>       } else { >> -        node->base.mm_nodes[0].start = 0; >> -        node->base.mm_nodes[0].size = node->base.base.num_pages; >> -        node->base.base.start = AMDGPU_BO_INVALID_OFFSET; >> +        node->mm_nodes[0].start = 0; >> +        node->mm_nodes[0].size = node->base.num_pages; >> +        node->base.start = AMDGPU_BO_INVALID_OFFSET; >>       } >>   -    *res = &node->base.base; >> +    *res = &node->base; >>       return 0; >>     err_free: >> @@ -191,12 +178,12 @@ static int amdgpu_gtt_mgr_new(struct >> ttm_resource_manager *man, >>   static void amdgpu_gtt_mgr_del(struct ttm_resource_manager *man, >>                      struct ttm_resource *res) >>   { >> -    struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(res); >> +    struct ttm_range_mgr_node *node = to_ttm_range_mgr_node(res); >>       struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man); >>         spin_lock(&mgr->lock); >> -    if (drm_mm_node_allocated(&node->base.mm_nodes[0])) >> -        drm_mm_remove_node(&node->base.mm_nodes[0]); >> +    if (drm_mm_node_allocated(&node->mm_nodes[0])) >> +        drm_mm_remove_node(&node->mm_nodes[0]); >>       spin_unlock(&mgr->lock); >>       atomic64_add(res->num_pages, &mgr->available); >>   @@ -228,14 +215,14 @@ uint64_t amdgpu_gtt_mgr_usage(struct >> ttm_resource_manager *man) >>   int amdgpu_gtt_mgr_recover(struct ttm_resource_manager *man) >>   { >>       struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man); >> -    struct amdgpu_gtt_node *node; >> +    struct ttm_range_mgr_node *node; >>       struct drm_mm_node *mm_node; >>       int r = 0; >>         spin_lock(&mgr->lock); >>       drm_mm_for_each_node(mm_node, &mgr->mm) { >> -        node = container_of(mm_node, typeof(*node), base.mm_nodes[0]); >> -        r = amdgpu_ttm_recover_gart(node->tbo); >> +        node = container_of(mm_node, typeof(*node), mm_nodes[0]); >> +        r = amdgpu_ttm_recover_gart(node->base.bo); >>           if (r) >>               break; >>       } >> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c >> b/drivers/gpu/drm/ttm/ttm_resource.c >> index 2431717376e7..7ff6194154fe 100644 >> --- a/drivers/gpu/drm/ttm/ttm_resource.c >> +++ b/drivers/gpu/drm/ttm/ttm_resource.c >> @@ -41,6 +41,7 @@ void ttm_resource_init(struct ttm_buffer_object *bo, >>       res->bus.offset = 0; >>       res->bus.is_iomem = false; >>       res->bus.caching = ttm_cached; >> +    res->bo = bo; >>   } >>   EXPORT_SYMBOL(ttm_resource_init); >>   diff --git a/include/drm/ttm/ttm_resource.h >> b/include/drm/ttm/ttm_resource.h >> index 140b6b9a8bbe..6d0b7a6d2169 100644 >> --- a/include/drm/ttm/ttm_resource.h >> +++ b/include/drm/ttm/ttm_resource.h >> @@ -171,6 +171,7 @@ struct ttm_resource { >>       uint32_t mem_type; >>       uint32_t placement; >>       struct ttm_bus_placement bus; >> +    struct ttm_buffer_object *bo; > > Not that I'm against this change by itself, but this bo pointer is not > refcounted, and therefore needs a description when it's needed and > why. What happens, for example when the resource is moved to a ghost > object, or the bo is killed while the resource is remaining on a lru > list (which I understand was one of the main purposes with > free-standing resources). Weak references need a guarantee that the > object they pointed to is alive. What is that guarantee? The basic idea is that we want to get rid of ghost objects and all the related workarounds in the mid term. But yes for the interim we probably need more logic here to make sure the BO is not destroyed. > > Also could we introduce new TTM structure members where they are first > used /referenced by TTM and not where they are used by amdgpu? Without > finding out in patch 3 that this member is needed to look up the bo > from a lru list the correct response to this patch would have been: > That bo is amdgpu-specific and needs to be in a driver private struct... That was indeed not supposed like this. The question I rather wanted to raise if it's ok to make the resource object fully depend on the BO for allocation/destruction? I've seen some code in the DG1 branch where a mock BO is used to allocate some resource, but that approach won't work with this here. On the other hand this fixes a long outstanding and very annoying problem we had. Christian. > > > Thanks, > > /Thomas > > >>   }; >>     /**