Message ID | 20231029230205.93277-16-dmitry.osipenko@collabora.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp1894883vqb; Sun, 29 Oct 2023 16:19:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGV0Rl+Bh7n0OXHJMZKDcaZxwIau2zRShJ2bFXbY7g4aR9Q6Q7Dihr+b92TvUdxs2F7yv9g X-Received: by 2002:a05:6808:9b5:b0:3b2:f15e:459f with SMTP id e21-20020a05680809b500b003b2f15e459fmr8477560oig.58.1698621551145; Sun, 29 Oct 2023 16:19:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698621551; cv=none; d=google.com; s=arc-20160816; b=JE8LozVmRJEhGgV52J/xpxoiyKmlU0lhDfpQRwlfd1KLE5kwm/2FnJbNu8LUQCN7Ef V8yg3QL5Cv7qAMIoZtE9fsCF2ffqGfsallpBVpjDCv0igtS4tvZEHrbNN2VqoOnrD3tg aEVPmhdQ2/RTwyKyKXSDTdSzwDQki1ltCMNdqeRDjZ3c1uPY3H9Rp8aPqC8zJBaSs6jU 1oupry5Y+nkOO3JH3SV2mrzg5skCLYEXmkMM1AwMnpbfSvuvKlJvli6BhpkOcS2/N1x1 T/nsKciE+HGRiZn2izofIvj5D/b9nysMJesvZzS1tgiElY9lODKpb0HQ63eYU+GC6fkS W6Fw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=UAG9Oxtlor+kMy7BHyI2oEOYK6ntX/ScZXM1CZKO6jA=; fh=5cel2jD5h+yPMXVwxbomyVhwojHUqATy6nFjd4aOh4o=; b=woppkWAqDX29FfJBMnkjgUZC4CmMz0fs0lwUdi+QxdhpzTF8xy+Jdd1GnxRcVMi09x QzsXEHViRlOF1v6y7fPZQbH5r4k3es7vnrjTom45w2k32ddSV/1XNDV5+FfUi4ph4gL5 AzdgjXA2B+Z5t0LC4BmXyl1cOypdLJHIl5GONByZIIwyrIyI4wMccB9GV1qQnjoHYRKk zxAJPHAv7EHrUgLqxa/G9vnuW4kP0JdqySUja1QVQ5QcwXsjc2oM8cetkMyWw8gb55ot cLAq+xU9HV3ejA8Rk8rjjJPKmnlz0ldVRUsZ7Wj3m15/kQQjbfO9jVUF2tAYTb8iqG6m YVUQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b="C7/+29RS"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id t189-20020a6381c6000000b005b96af23fe8si2144052pgd.424.2023.10.29.16.19.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 29 Oct 2023 16:19:11 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b="C7/+29RS"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 08E5F80B2324; Sun, 29 Oct 2023 16:19:10 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231768AbjJ2XTH (ORCPT <rfc822;zxc52fgh@gmail.com> + 31 others); Sun, 29 Oct 2023 19:19:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41122 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232024AbjJ2XSg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 29 Oct 2023 19:18:36 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 104EF7D88 for <linux-kernel@vger.kernel.org>; Sun, 29 Oct 2023 16:04:17 -0700 (PDT) Received: from workpc.. (109-252-153-31.dynamic.spd-mgts.ru [109.252.153.31]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: dmitry.osipenko) by madras.collabora.co.uk (Postfix) with ESMTPSA id 0BF0066073A3; Sun, 29 Oct 2023 23:02:47 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1698620569; bh=kigW7BmHwtRjkP5CdmHcrhQZWaYuJgNAqNfW+QOmSH4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=C7/+29RSZH8OEUufwdd12Q+/++94aRdOFa3S9xWzEYFLGkzFcnMN6fNYtr5s5Skoz FwA2Y4eXouplLhsbWhTKAf8M6OmHf2DQpe+zE35CUqJ99as5cm4btqm31Jfyg1SBHj PAOt/mthcB43f601TpiBsKsVIB6oRpTf6opv4yL9nvZCOCjIRXvIdOEvdyAqEoIAFx +Lju3LY5F3iRQfwllUvPlmJAp6atpCEBwoTkIxUGA+a3LYpl99Rh2ZgJUBnKN/sGkb KNH89cB7lHz1PF6GeRuDRJhB7fGsThMxBx1XpMVbZBPNwrviZIslPNzCgeReO0oMDn J2ajECiW8XRcQ== From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: David Airlie <airlied@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Gurchetan Singh <gurchetansingh@chromium.org>, Chia-I Wu <olvaffe@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, Qiang Yu <yuq825@gmail.com>, Steven Price <steven.price@arm.com>, Boris Brezillon <boris.brezillon@collabora.com>, Emma Anholt <emma@anholt.net>, Melissa Wen <mwen@igalia.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel@collabora.com, virtualization@lists.linux-foundation.org Subject: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages Date: Mon, 30 Oct 2023 02:01:54 +0300 Message-ID: <20231029230205.93277-16-dmitry.osipenko@collabora.com> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231029230205.93277-1-dmitry.osipenko@collabora.com> References: <20231029230205.93277-1-dmitry.osipenko@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Sun, 29 Oct 2023 16:19:10 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781133791806528510 X-GMAIL-MSGID: 1781133791806528510 |
Series |
Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
|
|
Commit Message
Dmitry Osipenko
Oct. 29, 2023, 11:01 p.m. UTC
To simplify the drm-shmem refcnt handling, we're moving away from
the implicit get_pages() that is used by get_pages_sgt(). From now on
drivers will have to pin pages while they use sgt. Panfrost's shrinker
doesn't support swapping out BOs, hence pages are pinned and sgt is valid
as long as pages' use-count > 0.
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++
drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++----
2 files changed, 19 insertions(+), 4 deletions(-)
Comments
On Mon, 30 Oct 2023 02:01:54 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > To simplify the drm-shmem refcnt handling, we're moving away from > the implicit get_pages() that is used by get_pages_sgt(). From now on > drivers will have to pin pages while they use sgt. Panfrost's shrinker > doesn't support swapping out BOs, hence pages are pinned and sgt is valid > as long as pages' use-count > 0. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++ > drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++---- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 6b77d8cebcb2..bb9d43cf7c3c 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > } > } > kvfree(bo->sgts); > + > + drm_gem_shmem_put_pages(&bo->base); > } > > + if (!bo->is_heap && !obj->import_attach) > + drm_gem_shmem_put_pages(&bo->base); > + > drm_gem_shmem_free(&bo->base); > } > > @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > { > struct drm_gem_shmem_object *shmem; > struct panfrost_gem_object *bo; > + int err; > > /* Round up heap allocations to 2MB to keep fault handling simple */ > if (flags & PANFROST_BO_HEAP) > @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > bo->noexec = !!(flags & PANFROST_BO_NOEXEC); > bo->is_heap = !!(flags & PANFROST_BO_HEAP); > > + if (!bo->is_heap) { > + err = drm_gem_shmem_get_pages(shmem); Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we add a drm_gem_shmem_get_pages()? What we should do instead is add a drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we have in the driver (in panfrost_mmu_map()), and add drm_gem_shmem_put_pages() calls where they are missing (panfrost_mmu_unmap()). > + if (err) > + goto err_free; > + } > + > return bo; > + > +err_free: > + drm_gem_shmem_free(&bo->base); > + > + return ERR_PTR(err); > } > > struct drm_gem_object * > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 770dab1942c2..ac145a98377b 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > if (IS_ERR(pages[i])) { > ret = PTR_ERR(pages[i]); > pages[i] = NULL; > - goto err_pages; > + goto err_unlock; > } > } > > @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); > if (ret) > - goto err_pages; > + goto err_unlock; Feels like the panfrost_gem_mapping object should hold a ref on the BO pages, not the BO itself, because, ultimately, the user of the BO is the GPU. This matches what I was saying about moving get/put_pages() to panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes active, to want to take a pages ref, every time it becomes inactive, you should release the pages ref. > > ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0); > if (ret) > @@ -535,8 +535,6 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > > err_map: > sg_free_table(sgt); > -err_pages: > - drm_gem_shmem_put_pages_locked(&bo->base); > err_unlock: > dma_resv_unlock(obj->resv); > err_bo:
On 11/10/23 13:53, Boris Brezillon wrote: > Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we > add a drm_gem_shmem_get_pages()? What we should do instead is add a > drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we > have in the driver (in panfrost_mmu_map()), and add > drm_gem_shmem_put_pages() calls where they are missing > (panfrost_mmu_unmap()). > >> + if (err) >> + goto err_free; >> + } >> + >> return bo; >> + >> +err_free: >> + drm_gem_shmem_free(&bo->base); >> + >> + return ERR_PTR(err); >> } >> >> struct drm_gem_object * >> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c >> index 770dab1942c2..ac145a98377b 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >> if (IS_ERR(pages[i])) { >> ret = PTR_ERR(pages[i]); >> pages[i] = NULL; >> - goto err_pages; >> + goto err_unlock; >> } >> } >> >> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, >> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); >> if (ret) >> - goto err_pages; >> + goto err_unlock; > Feels like the panfrost_gem_mapping object should hold a ref on the BO > pages, not the BO itself, because, ultimately, the user of the BO is > the GPU. This matches what I was saying about moving get/put_pages() to > panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes > active, to want to take a pages ref, every time it becomes inactive, > you should release the pages ref. The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm unhappy with how icky it all becomes if unmap is made to put pages. Previously map() was implicitly allocating pages with get_sgt() and then pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is mapped when it's created by Panfrost, hence the actual lifetime of pages is kept unchanged by this patch. The implicit allocation is turned into explicit one, i.e. pages are explicitly allocated before BO is mapped.
On Thu, 23 Nov 2023 01:04:56 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/10/23 13:53, Boris Brezillon wrote: > > Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we > > add a drm_gem_shmem_get_pages()? What we should do instead is add a > > drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we > > have in the driver (in panfrost_mmu_map()), and add > > drm_gem_shmem_put_pages() calls where they are missing > > (panfrost_mmu_unmap()). > > > >> + if (err) > >> + goto err_free; > >> + } > >> + > >> return bo; > >> + > >> +err_free: > >> + drm_gem_shmem_free(&bo->base); > >> + > >> + return ERR_PTR(err); > >> } > >> > >> struct drm_gem_object * > >> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> index 770dab1942c2..ac145a98377b 100644 > >> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > >> if (IS_ERR(pages[i])) { > >> ret = PTR_ERR(pages[i]); > >> pages[i] = NULL; > >> - goto err_pages; > >> + goto err_unlock; > >> } > >> } > >> > >> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > >> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > >> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); > >> if (ret) > >> - goto err_pages; > >> + goto err_unlock; > > Feels like the panfrost_gem_mapping object should hold a ref on the BO > > pages, not the BO itself, because, ultimately, the user of the BO is > > the GPU. This matches what I was saying about moving get/put_pages() to > > panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes > > active, to want to take a pages ref, every time it becomes inactive, > > you should release the pages ref. > > The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm > unhappy with how icky it all becomes if unmap is made to put pages. Why, that's exactly what's supposed to happen. If you mmu_unmap(), that means you no longer need the pages ref you got. > > Previously map() was implicitly allocating pages with get_sgt() and then > pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is > mapped when it's created by Panfrost, hence the actual lifetime of pages > is kept unchanged by this patch. But the whole point of making it explicit is to control when pages are needed or not, isn't it. The fact we mmu_map() the BO at open time, and keep it mapped until it's not longer referenced is an implementation choice, and I don't think having pages_put() in mmu_unmap() changes that. > The implicit allocation is turned into > explicit one, i.e. pages are explicitly allocated before BO is mapped. >
On 11/23/23 12:05, Boris Brezillon wrote: > On Thu, 23 Nov 2023 01:04:56 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 11/10/23 13:53, Boris Brezillon wrote: >>> Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we >>> add a drm_gem_shmem_get_pages()? What we should do instead is add a >>> drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we >>> have in the driver (in panfrost_mmu_map()), and add >>> drm_gem_shmem_put_pages() calls where they are missing >>> (panfrost_mmu_unmap()). >>> >>>> + if (err) >>>> + goto err_free; >>>> + } >>>> + >>>> return bo; >>>> + >>>> +err_free: >>>> + drm_gem_shmem_free(&bo->base); >>>> + >>>> + return ERR_PTR(err); >>>> } >>>> >>>> struct drm_gem_object * >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> index 770dab1942c2..ac145a98377b 100644 >>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >>>> if (IS_ERR(pages[i])) { >>>> ret = PTR_ERR(pages[i]); >>>> pages[i] = NULL; >>>> - goto err_pages; >>>> + goto err_unlock; >>>> } >>>> } >>>> >>>> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >>>> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, >>>> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); >>>> if (ret) >>>> - goto err_pages; >>>> + goto err_unlock; >>> Feels like the panfrost_gem_mapping object should hold a ref on the BO >>> pages, not the BO itself, because, ultimately, the user of the BO is >>> the GPU. This matches what I was saying about moving get/put_pages() to >>> panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes >>> active, to want to take a pages ref, every time it becomes inactive, >>> you should release the pages ref. >> >> The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm >> unhappy with how icky it all becomes if unmap is made to put pages. > > Why, that's exactly what's supposed to happen. If you mmu_unmap(), that > means you no longer need the pages ref you got. The drm_gem_shmem_purge() frees the pages. If mmu_unmap() frees pages too, then it becomes odd for drm_gem_shmem_purge() that it needs to free pages that were already freed. >> Previously map() was implicitly allocating pages with get_sgt() and then >> pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is >> mapped when it's created by Panfrost, hence the actual lifetime of pages >> is kept unchanged by this patch. > > But the whole point of making it explicit is to control when pages are > needed or not, isn't it. The fact we mmu_map() the BO at open time, and > keep it mapped until it's not longer referenced is an implementation > choice, and I don't think having pages_put() in mmu_unmap() changes > that. Previously, when the last mmu_unmap() was done, the pages were not released. If you'll make unmap to put pages, then you can't map BO again because pages are released by the last put() of unmap. In order to keep the old pages allocation logic unchanged, the pages must be referenced while BO is alive, not while mapping is alive. Technically, the code can be changed to put pages on unmap. This requires adding special quirk to drm_gem_shmem_purge() and then for Panfrost pages should have the same lifetime as BO, hence why bother? diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 5ee98b6f0c94..5492610802a1 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -600,7 +600,9 @@ drm_gem_shmem_shrinker_put_pages_locked(struct drm_gem_shmem_object *shmem) if (shmem->evicted) return; - drm_gem_shmem_free_pages(shmem); + if (refcount_read(&shmem->pages_use_count)) + drm_gem_shmem_free_pages(shmem); + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); } @@ -608,7 +610,8 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) { struct drm_gem_object *obj = &shmem->base; - drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); + if (refcount_read(&shmem->pages_use_count)) + drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); drm_gem_shmem_shrinker_put_pages_locked(shmem); drm_gem_free_mmap_offset(obj); diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index a6128e32f303..499964c43a7b 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -41,9 +41,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) drm_gem_shmem_put_pages(&bo->base); } - if (!bo->is_heap && !obj->import_attach) - drm_gem_shmem_put_pages(&bo->base); - drm_gem_shmem_free(&bo->base); } @@ -302,12 +299,6 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) bo->noexec = !!(flags & PANFROST_BO_NOEXEC); bo->is_heap = !!(flags & PANFROST_BO_HEAP); - if (!bo->is_heap) { - err = drm_gem_shmem_get_pages(shmem); - if (err) - goto err_free; - } - return bo; err_free: diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 01cd97011ea5..4ed4ba5df420 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -335,9 +335,13 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) prot |= IOMMU_NOEXEC; if (!obj->import_attach) { - ret = drm_gem_shmem_pin(shmem); + ret = drm_gem_shmem_get_pages(shmem); if (ret) return ret; + + ret = drm_gem_shmem_pin(shmem); + if (ret) + goto put_pages; } sgt = drm_gem_shmem_get_pages_sgt(shmem); @@ -349,9 +353,18 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT, prot, sgt); mapping->active = true; + + if (!obj->import_attach) + drm_gem_shmem_unpin(shmem); + + return 0; + unpin: if (!obj->import_attach) drm_gem_shmem_unpin(shmem); +put_pages: + if (!obj->import_attach) + drm_gem_shmem_put_pages(shmem); return ret; } @@ -360,6 +373,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping) { struct panfrost_gem_object *bo = mapping->obj; struct drm_gem_object *obj = &bo->base.base; + struct drm_gem_shmem_object *shmem = &bo->base; struct panfrost_device *pfdev = to_panfrost_device(obj->dev); struct io_pgtable_ops *ops = mapping->mmu->pgtbl_ops; u64 iova = mapping->mmnode.start << PAGE_SHIFT; @@ -389,6 +403,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping) panfrost_mmu_flush_range(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT, len); mapping->active = false; + + if (!bo->is_heap && !obj->import_attach) + drm_gem_shmem_put_pages_locked(shmem); } static void mmu_tlb_inv_context_s1(void *cookie)
On Thu, 23 Nov 2023 15:24:32 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/23/23 12:05, Boris Brezillon wrote: > > On Thu, 23 Nov 2023 01:04:56 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> On 11/10/23 13:53, Boris Brezillon wrote: > >>> Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why > >>> should we add a drm_gem_shmem_get_pages()? What we should do > >>> instead is add a drm_gem_shmem_get_pages() for each > >>> drm_gem_shmem_get_pages_sgt() we have in the driver (in > >>> panfrost_mmu_map()), and add drm_gem_shmem_put_pages() calls > >>> where they are missing (panfrost_mmu_unmap()). > >>> > >>>> + if (err) > >>>> + goto err_free; > >>>> + } > >>>> + > >>>> return bo; > >>>> + > >>>> +err_free: > >>>> + drm_gem_shmem_free(&bo->base); > >>>> + > >>>> + return ERR_PTR(err); > >>>> } > >>>> > >>>> struct drm_gem_object * > >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c index > >>>> 770dab1942c2..ac145a98377b 100644 --- > >>>> a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ > >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -504,7 +504,7 @@ > >>>> static int panfrost_mmu_map_fault_addr(struct panfrost_device > >>>> *pfdev, int as, if (IS_ERR(pages[i])) { ret = PTR_ERR(pages[i]); > >>>> pages[i] = NULL; > >>>> - goto err_pages; > >>>> + goto err_unlock; > >>>> } > >>>> } > >>>> > >>>> @@ -512,7 +512,7 @@ static int > >>>> panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int > >>>> as, ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > >>>> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); if (ret) > >>>> - goto err_pages; > >>>> + goto err_unlock; > >>> Feels like the panfrost_gem_mapping object should hold a ref on > >>> the BO pages, not the BO itself, because, ultimately, the user of > >>> the BO is the GPU. This matches what I was saying about moving > >>> get/put_pages() to panfrost_mmu_map/unmap(): everytime a > >>> panfrost_gem_mapping becomes active, to want to take a pages ref, > >>> every time it becomes inactive, you should release the pages ref. > >>> > >> > >> The panfrost_mmu_unmap() is also used by shrinker when BO is > >> purged. I'm unhappy with how icky it all becomes if unmap is made > >> to put pages. > > > > Why, that's exactly what's supposed to happen. If you mmu_unmap(), > > that means you no longer need the pages ref you got. > > The drm_gem_shmem_purge() frees the pages. If mmu_unmap() frees pages > too, then it becomes odd for drm_gem_shmem_purge() that it needs to > free pages that were already freed. Hm, I didn't consider the mmu_unmap() call in the eviction path. > > >> Previously map() was implicitly allocating pages with get_sgt() > >> and then pages were implicitly released by drm_gem_shmem_free(). A > >> non-heap BO is mapped when it's created by Panfrost, hence the > >> actual lifetime of pages is kept unchanged by this patch. > > > > But the whole point of making it explicit is to control when pages > > are needed or not, isn't it. The fact we mmu_map() the BO at open > > time, and keep it mapped until it's not longer referenced is an > > implementation choice, and I don't think having pages_put() in > > mmu_unmap() changes that. > > Previously, when the last mmu_unmap() was done, the pages were not > released. > > If you'll make unmap to put pages, then you can't map BO again > because pages are released by the last put() of unmap. Well, you could, if panfrost_gem_mapping_get() was not only returning an existing mapping, but was also creating one when none exist. But you're right, it messes up with the shmem shrinker and also changes the way we are doing things now. > In order to > keep the old pages allocation logic unchanged, the pages must be > referenced while BO is alive, not while mapping is alive. Correct. > > Technically, the code can be changed to put pages on unmap. This > requires adding special quirk to drm_gem_shmem_purge() and then for > Panfrost pages should have the same lifetime as BO, hence why bother? No, we certainly don't want to do that.
On Mon, 30 Oct 2023 02:01:54 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > To simplify the drm-shmem refcnt handling, we're moving away from > the implicit get_pages() that is used by get_pages_sgt(). From now on > drivers will have to pin pages while they use sgt. Panfrost's shrinker > doesn't support swapping out BOs, hence pages are pinned and sgt is valid > as long as pages' use-count > 0. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++ > drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++---- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 6b77d8cebcb2..bb9d43cf7c3c 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > } > } > kvfree(bo->sgts); > + > + drm_gem_shmem_put_pages(&bo->base); > } > > + if (!bo->is_heap && !obj->import_attach) > + drm_gem_shmem_put_pages(&bo->base); > + > drm_gem_shmem_free(&bo->base); > } > > @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > { > struct drm_gem_shmem_object *shmem; > struct panfrost_gem_object *bo; > + int err; > > /* Round up heap allocations to 2MB to keep fault handling simple */ > if (flags & PANFROST_BO_HEAP) > @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > bo->noexec = !!(flags & PANFROST_BO_NOEXEC); > bo->is_heap = !!(flags & PANFROST_BO_HEAP); > > + if (!bo->is_heap) { > + err = drm_gem_shmem_get_pages(shmem); I really hate the fact we request pages here while we call panfrost_mmu_map() in panfrost_gem_open(), because ultimately, pages are requested for the MMU mapping. Also hate the quirk we have in shmem to call free_pages() instead of put_pages_locked() when the BO refcount dropped to zero, and I was hoping we could get rid of it at some point by teaching drivers to request pages when they actually need it instead of tying pages lifetime to the GEM object lifetime. Maybe what we should do instead is move the get/put_pages() in panfrost_mmu_map/unmap() (as I suggested), but have a special mapping panfrost_mmu_evict/restore() helpers that kill/restore the MMU mappings without releasing/acquiring the pages ref.
On Thu, 23 Nov 2023 15:48:48 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Mon, 30 Oct 2023 02:01:54 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > To simplify the drm-shmem refcnt handling, we're moving away from > > the implicit get_pages() that is used by get_pages_sgt(). From now on > > drivers will have to pin pages while they use sgt. Panfrost's shrinker > > doesn't support swapping out BOs, hence pages are pinned and sgt is valid > > as long as pages' use-count > 0. > > > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++ > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++---- > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > > index 6b77d8cebcb2..bb9d43cf7c3c 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > > @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > > } > > } > > kvfree(bo->sgts); > > + > > + drm_gem_shmem_put_pages(&bo->base); > > } > > > > + if (!bo->is_heap && !obj->import_attach) > > + drm_gem_shmem_put_pages(&bo->base); > > + > > drm_gem_shmem_free(&bo->base); > > } > > > > @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > > { > > struct drm_gem_shmem_object *shmem; > > struct panfrost_gem_object *bo; > > + int err; > > > > /* Round up heap allocations to 2MB to keep fault handling simple */ > > if (flags & PANFROST_BO_HEAP) > > @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > > bo->noexec = !!(flags & PANFROST_BO_NOEXEC); > > bo->is_heap = !!(flags & PANFROST_BO_HEAP); > > > > + if (!bo->is_heap) { > > + err = drm_gem_shmem_get_pages(shmem); > > I really hate the fact we request pages here while we call > panfrost_mmu_map() in panfrost_gem_open(), because ultimately, pages > are requested for the MMU mapping. Also hate the quirk we have in shmem > to call free_pages() instead of put_pages_locked() when the BO refcount > dropped to zero, and I was hoping we could get rid of it at some point > by teaching drivers to request pages when they actually need it instead > of tying pages lifetime to the GEM object lifetime. > > Maybe what we should do instead is move the get/put_pages() in > panfrost_mmu_map/unmap() (as I suggested), but have a special mapping > panfrost_mmu_evict/restore() helpers that kill/restore the MMU mappings > without releasing/acquiring the pages ref. Okay, so I played with your branch and did what I suggested here ^. The end result is available here [1]. I also split this patch in two: - A fix for the error path in panfrost_mmu_map_fault_addr() [2] - The explicit get/put_pages() stuff with pages ref owned by the panfrost_gem_mapping object [3] [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commits/virtio-gpu-shrinker-v18 [2]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/9d499e971fdae4d6e52f8871ca27c24b2a2c43d6 [3]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/ba3de65bf1cf0ca95710e743ec85ca67ff1aa223
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 6b77d8cebcb2..bb9d43cf7c3c 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) } } kvfree(bo->sgts); + + drm_gem_shmem_put_pages(&bo->base); } + if (!bo->is_heap && !obj->import_attach) + drm_gem_shmem_put_pages(&bo->base); + drm_gem_shmem_free(&bo->base); } @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) { struct drm_gem_shmem_object *shmem; struct panfrost_gem_object *bo; + int err; /* Round up heap allocations to 2MB to keep fault handling simple */ if (flags & PANFROST_BO_HEAP) @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) bo->noexec = !!(flags & PANFROST_BO_NOEXEC); bo->is_heap = !!(flags & PANFROST_BO_HEAP); + if (!bo->is_heap) { + err = drm_gem_shmem_get_pages(shmem); + if (err) + goto err_free; + } + return bo; + +err_free: + drm_gem_shmem_free(&bo->base); + + return ERR_PTR(err); } struct drm_gem_object * diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 770dab1942c2..ac145a98377b 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, if (IS_ERR(pages[i])) { ret = PTR_ERR(pages[i]); pages[i] = NULL; - goto err_pages; + goto err_unlock; } } @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, ret = sg_alloc_table_from_pages(sgt, pages + page_offset, NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); if (ret) - goto err_pages; + goto err_unlock; ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0); if (ret) @@ -535,8 +535,6 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, err_map: sg_free_table(sgt); -err_pages: - drm_gem_shmem_put_pages_locked(&bo->base); err_unlock: dma_resv_unlock(obj->resv); err_bo: