Received: by 2002:a05:6512:2355:0:0:0:0 with SMTP id p21csp5526507lfu; Mon, 28 Mar 2022 16:00:49 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxnKX4zHtSz7jGNF7RHrl1H9SCnMLWb6EXnXoyFxpvFIRMCug/ufkULzc4h2ydqhuw5D1G9 X-Received: by 2002:a65:55cd:0:b0:386:861:8a3c with SMTP id k13-20020a6555cd000000b0038608618a3cmr11790030pgs.278.1648508449251; Mon, 28 Mar 2022 16:00:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1648508449; cv=none; d=google.com; s=arc-20160816; b=hah/3wZBybTUzjLOmn7JgEolgFSfrRB/wfWkp6xEHAB2Uxx+Dht4G5coUIMUEOZTSQ cNOd3DnuIpur9II6bUwMox7rwiaTQTyy2FMm85YLlPg95ufD0ULb8/IBw5bVUDx5PWYG PlNbpAUZ7Z5fBiDxbbFNJzVe1uZSOjbynv+/ml3hE/w9lAoZR1D5CBriT53fM6DFeARP p7x+KCkJRDxW85wXZ96o5kUjh1SNJTh0uS5MKUzAcluLK6gnj35Hp340mRZ774+vJElA lm+Ao/a5RHlkG+3i4/nUW1FNwsX/9fiR31J8M/fV8BPDun3ZG4o9nWxf+djPSyRk6LOH y40Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=8WaH8oTp9zQ4RiMod88tKNM8FNGzolc7NDGliqXEyFM=; b=QxrqOfXepTGfi8DZKZNOJfsXPWtIAddxuwoU4aEi0Op6ssgDdWT2q/2zsgsvR6GBR2 2Mcx3vCpZnXZkRi/kYuWiWyZcSOKJHrbuvEaTZD9pqCs5RCO3A9h6jycXxwdSMmp9+Vs WQ57IYJno1m8lg3umF/Dda6Q8xnTYcoeb8UuUaL8IAlH/3zt+SxO5R4q3VJeQilxUkt3 QUG2oGaRGMfhcfyk5GaUk3JFdjm4T5P8r0Y57grshBblUlrw4gpY5kWC2ByUKt3cdm/I ovlLbAb8naeKsqI1+2qo0HZ73nbfvaTbl6Vv36HXrJNxF155p5gT5L6kXUOsZuD9VV20 zF6w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=RA1LFZly; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [23.128.96.19]) by mx.google.com with ESMTPS id l24-20020a656818000000b003816043ef40si14822378pgt.309.2022.03.28.16.00.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Mar 2022 16:00:49 -0700 (PDT) Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=RA1LFZly; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id EF831287A35; Mon, 28 Mar 2022 15:00:02 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245121AbiC1SaX (ORCPT + 99 others); Mon, 28 Mar 2022 14:30:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50054 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242272AbiC1SaU (ORCPT ); Mon, 28 Mar 2022 14:30:20 -0400 Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8DDE813E20 for ; Mon, 28 Mar 2022 11:28:37 -0700 (PDT) Received: by mail-ej1-x635.google.com with SMTP id dr20so30441999ejc.6 for ; Mon, 28 Mar 2022 11:28:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=8WaH8oTp9zQ4RiMod88tKNM8FNGzolc7NDGliqXEyFM=; b=RA1LFZlyqYnQKlZhfG5eW4EytFq6FXGfsjb2/DOkWkAoisIvQquLsZTwb8nBtNlht8 ZHqiJ/MOFhcB+HyYDg4DOrCFHvXKNreT5wQEPJ7PzPnMoI2QrObdeAd08uOLLZhCsV8Y oo/fwNelFB8E06amjBdYhwsR2VhATbyd3kx12XlxxP1sGczHl+YWk9N47mzb48HohD17 d9HYqBqtvUEw1TYZH7O9tEkaSoRyqnabxydRHWLDA8W6lAvtHgHxiIUB7dc49IG+TBgi G8ykTzQvXQ9aD4+tQHre6cRAhm0yoObw1rfkn0AAPUuFF1ZU/BUjCdEdRxBpKxz80uJg RymA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=8WaH8oTp9zQ4RiMod88tKNM8FNGzolc7NDGliqXEyFM=; b=MgNrm1we3zW8jHnqv1KssFEi33/HF1GlhdIdHUFQapFKfFNcLHvOzyUhiEPOf0FHup SWyCVQ5oMSy5RciTtDsWsrvmQPzum7iGI3QFECUZGZZ70GqgLRHrPAQ7Yl4voFUk5aVH 5kUmLMAKSTmipqAnClSCNeFr43bZz+2ZGu1Gtn5Mdfwa+BksNcDcqRujOywLiABGRjGJ MZzMQc83WZf53H1ZqtBsrDM3BE+4sZfk/nDx7azrvJWrm0CXrC0anqWsgkQoPzQopCqH iew0qhM2nuNEgByT5kFLeHxAVoVXa1kVjKI8Avv7Z3bpK+rYFrTOFY+TaYnsR8gyY7zR Zssg== X-Gm-Message-State: AOAM533Upv2lgmiOzwu9WdMUfc2zJhJw+1gJAOsRN5Sebtmu1tm/la5c +xiZcFpgWOVelEoq5eYGqx5LbaS3vyoC2SP1LayXhg== X-Received: by 2002:a17:906:9754:b0:6da:7d72:1353 with SMTP id o20-20020a170906975400b006da7d721353mr29368499ejy.273.1648492115577; Mon, 28 Mar 2022 11:28:35 -0700 (PDT) MIME-Version: 1.0 References: <20220328035951.1817417-1-tjmercier@google.com> <20220328035951.1817417-5-tjmercier@google.com> In-Reply-To: From: "T.J. Mercier" Date: Mon, 28 Mar 2022 11:28:24 -0700 Message-ID: Subject: Re: [RFC v4 4/8] dmabuf: heaps: export system_heap buffers with GPU cgroup charging To: "T.J. Mercier" , David Airlie , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Jonathan Corbet , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Christian Brauner , Hridya Valsaraju , Suren Baghdasaryan , Sumit Semwal , =?UTF-8?Q?Christian_K=C3=B6nig?= , Benjamin Gaignard , Liam Mark , Laura Abbott , Brian Starkey , John Stultz , Tejun Heo , Zefan Li , Johannes Weiner , Shuah Khan , Kalesh Singh , Kenny.Ho@amd.com, =?UTF-8?Q?Michal_Koutn=C3=BD?= , Shuah Khan , dri-devel@lists.freedesktop.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, cgroups@vger.kernel.org, linux-kselftest@vger.kernel.org Cc: Daniel Vetter Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL autolearn=no 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: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 28, 2022 at 7:36 AM Daniel Vetter wrote: > > On Mon, Mar 28, 2022 at 03:59:43AM +0000, T.J. Mercier wrote: > > From: Hridya Valsaraju > > > > All DMA heaps now register a new GPU cgroup device upon creation, and t= he > > system_heap now exports buffers associated with its GPU cgroup device f= or > > tracking purposes. > > > > Signed-off-by: Hridya Valsaraju > > Signed-off-by: T.J. Mercier > > > > --- > > v3 changes > > Use more common dual author commit message format per John Stultz. > > > > v2 changes > > Move dma-buf cgroup charge transfer from a dma_buf_op defined by every > > heap to a single dma-buf function for all heaps per Daniel Vetter and > > Christian K=C3=B6nig. > > Apologies for being out of the loop quite a bit. I scrolled through this > all and I think it looks good to get going. > > The only thing I have is whether we should move the cgroup controllers ou= t > of dma-buf heaps, since that's rather android centric. E.g. > - a system gpucg_device which is used by all the various single page > allocators (dma-buf heap but also shmem helpers and really anything > else) > - same for cma, again both for dma-buf heaps and also for the gem cma > helpers in drm Thanks Daniel, in general that makes sense to me as an approach to making this more universal. However for the Android case I'm not sure if the part about a single system gpucg_device would be sufficient, because there are at least 12 different graphics related heaps that could potentially be accounted/limited differently. [1] So that raises the question of how fine grained we want this to be... I tend towards separating them all, but I haven't formed a strong opinion about this at the moment. It sounds like you are in favor of a smaller, more rigidly defined set of them? Either way, we need to add code for accounting at points where we know memory is specifically for graphics use and not something else right? (I.E. Whether it is a dma-buf heap or somewhere like drm_gem_object_init.) So IIUC the only question is what to use for the gpucg_device(s) at these locations. [1] https://cs.android.com/android/platform/superproject/+/master:hardware/= google/graphics/common/libion/ion.cpp;l=3D39-50 > > Otherwise this will only work on non-upstream android where gpu drivers > allocate everything from dma-buf heap. If you use something like the x86 > android project with mesa drivers, then driver-internal buffers will be > allocated through gem and not through dma-buf heaps. Or at least I think > that's how it works. > > But also meh, we can fix this fairly easily later on by adding these > standard gpucg_dev somwehere with a bit of kerneldoc. This is what I was thinking would happen next, but IDK if anyone sees a more central place to do this type of use-specific accounting. > > Anyway has my all my ack, but don't count this as my in-depth review :-) > -Daniel Thanks again for taking a look! > > > --- > > drivers/dma-buf/dma-heap.c | 27 +++++++++++++++++++++++++++ > > drivers/dma-buf/heaps/system_heap.c | 3 +++ > > include/linux/dma-heap.h | 11 +++++++++++ > > 3 files changed, 41 insertions(+) > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > > index 8f5848aa144f..885072427775 100644 > > --- a/drivers/dma-buf/dma-heap.c > > +++ b/drivers/dma-buf/dma-heap.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include > > +#include > > #include > > #include > > #include > > @@ -31,6 +32,7 @@ > > * @heap_devt heap device node > > * @list list head connecting to list of heaps > > * @heap_cdev heap char device > > + * @gpucg_dev gpu cgroup device for memory accounting > > * > > * Represents a heap of memory from which buffers can be made. > > */ > > @@ -41,6 +43,9 @@ struct dma_heap { > > dev_t heap_devt; > > struct list_head list; > > struct cdev heap_cdev; > > +#ifdef CONFIG_CGROUP_GPU > > + struct gpucg_device gpucg_dev; > > +#endif > > }; > > > > static LIST_HEAD(heap_list); > > @@ -216,6 +221,26 @@ const char *dma_heap_get_name(struct dma_heap *hea= p) > > return heap->name; > > } > > > > +#ifdef CONFIG_CGROUP_GPU > > +/** > > + * dma_heap_get_gpucg_dev() - get struct gpucg_device for the heap. > > + * @heap: DMA-Heap to get the gpucg_device struct for. > > + * > > + * Returns: > > + * The gpucg_device struct for the heap. NULL if the GPU cgroup contro= ller is > > + * not enabled. > > + */ > > +struct gpucg_device *dma_heap_get_gpucg_dev(struct dma_heap *heap) > > +{ > > + return &heap->gpucg_dev; > > +} > > +#else /* CONFIG_CGROUP_GPU */ > > +struct gpucg_device *dma_heap_get_gpucg_dev(struct dma_heap *heap) > > +{ > > + return NULL; > > +} > > +#endif /* CONFIG_CGROUP_GPU */ > > + > > struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_i= nfo) > > { > > struct dma_heap *heap, *h, *err_ret; > > @@ -288,6 +313,8 @@ struct dma_heap *dma_heap_add(const struct dma_heap= _export_info *exp_info) > > list_add(&heap->list, &heap_list); > > mutex_unlock(&heap_list_lock); > > > > + gpucg_register_device(dma_heap_get_gpucg_dev(heap), exp_info->nam= e); > > + > > return heap; > > > > err2: > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heap= s/system_heap.c > > index ab7fd896d2c4..752a05c3cfe2 100644 > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -395,6 +395,9 @@ static struct dma_buf *system_heap_allocate(struct = dma_heap *heap, > > exp_info.ops =3D &system_heap_buf_ops; > > exp_info.size =3D buffer->len; > > exp_info.flags =3D fd_flags; > > +#ifdef CONFIG_CGROUP_GPU > > + exp_info.gpucg_dev =3D dma_heap_get_gpucg_dev(heap); > > +#endif > > exp_info.priv =3D buffer; > > dmabuf =3D dma_buf_export(&exp_info); > > if (IS_ERR(dmabuf)) { > > diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h > > index 0c05561cad6e..e447a61d054e 100644 > > --- a/include/linux/dma-heap.h > > +++ b/include/linux/dma-heap.h > > @@ -10,6 +10,7 @@ > > #define _DMA_HEAPS_H > > > > #include > > +#include > > #include > > > > struct dma_heap; > > @@ -59,6 +60,16 @@ void *dma_heap_get_drvdata(struct dma_heap *heap); > > */ > > const char *dma_heap_get_name(struct dma_heap *heap); > > > > +/** > > + * dma_heap_get_gpucg_dev() - get a pointer to the struct gpucg_device= for the > > + * heap. > > + * @heap: DMA-Heap to retrieve gpucg_device for. > > + * > > + * Returns: > > + * The gpucg_device struct for the heap. > > + */ > > +struct gpucg_device *dma_heap_get_gpucg_dev(struct dma_heap *heap); > > + > > /** > > * dma_heap_add - adds a heap to dmabuf heaps > > * @exp_info: information needed to register this heap > > -- > > 2.35.1.1021.g381101b075-goog > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch