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=-5.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,URIBL_BLOCKED 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 2880EC07E9C for ; Tue, 6 Jul 2021 21:04:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0BCF461CA6 for ; Tue, 6 Jul 2021 21:04:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230292AbhGFVGs (ORCPT ); Tue, 6 Jul 2021 17:06:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35720 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230265AbhGFVGr (ORCPT ); Tue, 6 Jul 2021 17:06:47 -0400 Received: from mail-lf1-x135.google.com (mail-lf1-x135.google.com [IPv6:2a00:1450:4864:20::135]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9CB36C06175F for ; Tue, 6 Jul 2021 14:04:08 -0700 (PDT) Received: by mail-lf1-x135.google.com with SMTP id u18so150660lff.9 for ; Tue, 06 Jul 2021 14:04:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=GCr9dR1B9S9xq9AszxUseP+SiyvelPfjfJ3LYfvLk30=; b=YTZ+5+qz57rgaRvigWO+axhDE6L/fF5mjwph4prQN7ju5u0+amqQ6czi4/Lc1SYQgW NqJSaAtqLmUdy8IVyuzwtoZseQ/lsl+IiAJNIxZh8f8E29qlGqzpT8p9JvVVgB5V+ZuU o1Nij7KOqASObEvNoHQs+3O2lwNLhXoD7kzX5vwAGyjSOPo3gUsP+w6XVlsfAEfJ7k71 m75lkW/tKPnRDOYBIq5OqMw1O1CQAiYkIU2p/nWps+4ksrY9sjg430sZxv6g31tDjPxi NpzMR2/4Fi1nMHGF0FDovSfD/ybOh4GKFxE1OdRd63nznPPlJbMWSiI/zQdmDFVHwhP/ m1aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=GCr9dR1B9S9xq9AszxUseP+SiyvelPfjfJ3LYfvLk30=; b=eEy0UNoHtrhficV9U5MeqUqv7oxUXd4IMWKx4EsH2PjtYtnKhUGkfDXbrNowGDFWVD fV8rCDWH/HDvlK+B6DIpCsadHbbvkp5sS0ueGVV1KcHjnIiOF62SyjJnR+D7NxnTz/C0 wsMxj0Or6gAGpQtxGwZ8CopxSMKQi0uwI0yEjeyoeL9419e+JwybvoQASeK1RWHVCVbR +jEVjSl+n7E6AosEwu0gxVsS6Xb2vgWNUUhoifHp90GD9bFgaf2xscGuyMr7PBdRa4T8 WVMDnRmWVaSIjYLefEbMFhk2lQcjB17192ed2HvaTtpMTtdIOJCwBdlHM3P47JWZuvxk YNiw== X-Gm-Message-State: AOAM532YEbHjSH2R6aWiCNnKXfAWC+BJbgKRy6hFXWK5+uiTKoULBYgh lIG6YLrGOXjvHaMCYaBkPDjp4N4U9QszJIFM2eiRgw== X-Google-Smtp-Source: ABdhPJyQY/P067tXLBOneScrLN2H7JnhxkuJjO0A7ZDRwnec+6MHaVd2CnVvcnLxIBIeYHDuBCOWVBUBBIjPebRPw6o= X-Received: by 2002:a05:6512:22cc:: with SMTP id g12mr8444627lfu.535.1625605446875; Tue, 06 Jul 2021 14:04:06 -0700 (PDT) MIME-Version: 1.0 References: <20210630013421.735092-1-john.stultz@linaro.org> <20210630013421.735092-2-john.stultz@linaro.org> <6a472a24-a40f-1160-70dd-5cb9e9ae85f1@amd.com> In-Reply-To: <6a472a24-a40f-1160-70dd-5cb9e9ae85f1@amd.com> From: John Stultz Date: Tue, 6 Jul 2021 14:03:58 -0700 Message-ID: Subject: Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation To: =?UTF-8?Q?Christian_K=C3=B6nig?= Cc: lkml , Daniel Vetter , Sumit Semwal , Liam Mark , Chris Goldsworthy , Laura Abbott , Brian Starkey , Hridya Valsaraju , Suren Baghdasaryan , Sandeep Patil , Daniel Mentz , =?UTF-8?Q?=C3=98rjan_Eide?= , Robin Murphy , Ezequiel Garcia , Simon Ser , James Jones , linux-media , dri-devel Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 30, 2021 at 11:52 PM Christian K=C3=B6nig wrote: > > Am 01.07.21 um 00:24 schrieb John Stultz: > > On Wed, Jun 30, 2021 at 2:10 AM Christian K=C3=B6nig > > wrote: > >> Am 30.06.21 um 03:34 schrieb John Stultz: > >>> +static unsigned long page_pool_size; /* max size of the pool */ > >>> + > >>> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the drm page po= ol"); > >>> +module_param(page_pool_size, ulong, 0644); > >>> + > >>> +static atomic_long_t nr_managed_pages; > >>> + > >>> +static struct mutex shrinker_lock; > >>> +static struct list_head shrinker_list; > >>> +static struct shrinker mm_shrinker; > >>> + > >>> +/** > >>> + * drm_page_pool_set_max - Sets maximum size of all pools > >>> + * > >>> + * Sets the maximum number of pages allows in all pools. > >>> + * This can only be set once, and the first caller wins. > >>> + */ > >>> +void drm_page_pool_set_max(unsigned long max) > >>> +{ > >>> + if (!page_pool_size) > >>> + page_pool_size =3D max; > >>> +} > >>> + > >>> +/** > >>> + * drm_page_pool_get_max - Maximum size of all pools > >>> + * > >>> + * Return the maximum number of pages allows in all pools > >>> + */ > >>> +unsigned long drm_page_pool_get_max(void) > >>> +{ > >>> + return page_pool_size; > >>> +} > >> Well in general I don't think it is a good idea to have getters/setter= s > >> for one line functionality, similar applies to locking/unlocking the > >> mutex below. > >> > >> Then in this specific case what those functions do is to aid > >> initializing the general pool manager and that in turn should absolute= ly > >> not be exposed. > >> > >> The TTM pool manager exposes this as function because initializing the > >> pool manager is done in one part of the module and calculating the > >> default value for the pages in another one. But that is not something = I > >> would like to see here. > > So, I guess I'm not quite clear on what you'd like to see... > > > > Part of what I'm balancing here is the TTM subsystem normally sets a > > global max size, whereas the old ION pool didn't have caps (instead > > just relying on the shrinker when needed). > > So I'm trying to come up with a solution that can serve both uses. So > > I've got this drm_page_pool_set_max() function to optionally set the > > maximum value, which is called in the TTM initialization path or set > > the boot argument. But for systems that use the dmabuf system heap, > > but don't use TTM, no global limit is enforced. > > Yeah, exactly that's what I'm trying to prevent. > > See if we have the same functionality used by different use cases we > should not have different behavior depending on what drivers are loaded. > > Is it a problem if we restrict the ION pool to 50% of system memory as > well? If yes than I would rather drop the limit from TTM and only rely > on the shrinker there as well. Would having the default value as a config option (still overridable via boot argument) be an acceptable solution? Thanks again for the feedback! thanks -john