Received: by 2002:a05:6358:11c7:b0:104:8066:f915 with SMTP id i7csp1064461rwl; Wed, 12 Apr 2023 07:51:44 -0700 (PDT) X-Google-Smtp-Source: AKy350aPrqwdOIr0bGwvBGNvfa46znE3qr6KPMynf56nCug1vTZkXatOY0zyDtnxVg6675og+6ZS X-Received: by 2002:a17:906:b044:b0:94a:460a:7c11 with SMTP id bj4-20020a170906b04400b0094a460a7c11mr6455142ejb.41.1681311104691; Wed, 12 Apr 2023 07:51:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681311104; cv=none; d=google.com; s=arc-20160816; b=b88x4Qa4CjeWJYu9Zxkeq7+QeaIfUmHM9npEzE73CQoWCG10eBTut8qOwWsYCORfVT uJ/W/dTNdQJJE00hH7ZWfRKDwOqESbOUTGVoaz4FbmIMa8iSnaJ+ZM1Czsn6gHiwgy5S t93i0lfvvqP0uAdIENMUyEOenbKLrwHQj1HdjS2megHnSHQQN/KRSPgfiz28qGE7P4RZ g+zJia4fntcEDF3lfoR79jrhMAcUrWbQIUCFHJDki9s0uy+12UzBa809I5+pNzYlrDPW Z42RtT6LQBvdHomuWrTTXAEo68gCzswOxqE6/Q/fbGKZwFX2SFoQqWIvO2aOP80LsX3u 4QMA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to :organization:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:dkim-signature; bh=pwd13cnvdrryAulwgbyw+bM9RMicRmM+14Vb51JazNQ=; b=oDyJvbpobAej7td4TEReoFWROUXmdy/tQrv6vLsJ6dUzkrYJqVbPZqXHXVlPGzXnVG cZrjTdF3p1SaltmSF1B4kx7g7DQKzytcdEToHu+SHQUmvYzT5KkE5veCs32e8LYSfAtU cMNuK1rgF1OswXoxqUb5wcXx2DZe4GG0ojF81b0To79lkbjRrgooT91CcEUQYbrdlux2 gcWbBltpF9wLRxirX8PoydBkG8lA+MYNJCKqKx5D67WZmPK+BTIpsRnivk+qZDLhXUDX 9RhttLiqrDYpzCQ7w/rGsrobGLmhS6w8G095N/rhIZoTNszlKCkj/JzkBLK1qW7m5sT9 Gs1Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KYSMBwVh; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id fy33-20020a1709069f2100b0094a9b1b8775si2093667ejc.105.2023.04.12.07.51.19; Wed, 12 Apr 2023 07:51:44 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KYSMBwVh; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231223AbjDLOm5 (ORCPT + 99 others); Wed, 12 Apr 2023 10:42:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44976 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230063AbjDLOmz (ORCPT ); Wed, 12 Apr 2023 10:42:55 -0400 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D6F0E65B7; Wed, 12 Apr 2023 07:42:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1681310573; x=1712846573; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=gXVOammf4Est891UaXJ7EnQKB4MFgzNaMuGrB5odaG8=; b=KYSMBwVhT5gMUGfarxgmT9uDTjYTv9ISP6upXsNfRq2YbqGDY/KobX3k +CQJpnsDJcym106DGXR9EFQNOT2dmUTc1mTGvApWPJgK+ceCgwq9rZzww 5SwFVuG434PunVoCxUyv2gPwDbJPCTDeo3y6h2QV3ie7dqYIkZBDCIB8C CJHJ4SRmSSQ8ara3G9hsrskCp25spvkf7dxslenJbfQc7xvROv3sjBB23 88iMeyygDyD2FrIsPS5ZzO/3LJRCf4+Rv8qkhegngI9nSXXl++spmOsth RFlreW9b54+PcUhwHTIKe3iZ/fdc7CnqiPbnzsauhzENWOijaEai89OfZ g==; X-IronPort-AV: E=McAfee;i="6600,9927,10678"; a="342671312" X-IronPort-AV: E=Sophos;i="5.98,339,1673942400"; d="scan'208";a="342671312" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2023 07:42:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10678"; a="719395174" X-IronPort-AV: E=Sophos;i="5.98,339,1673942400"; d="scan'208";a="719395174" Received: from amurkovx-mobl.ger.corp.intel.com (HELO [10.213.229.123]) ([10.213.229.123]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2023 07:42:48 -0700 Message-ID: <29a8d9aa-c6ea-873f-ce0b-fb8199b13068@linux.intel.com> Date: Wed, 12 Apr 2023 15:42:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v3 6/7] drm: Add fdinfo memory stats Content-Language: en-US To: Rob Clark , dri-devel@lists.freedesktop.org Cc: linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, Boris Brezillon , Christopher Healy , Emil Velikov , Rob Clark , David Airlie , Daniel Vetter , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Jonathan Corbet , "open list:DOCUMENTATION" , open list References: <20230411225725.2032862-1-robdclark@gmail.com> <20230411225725.2032862-7-robdclark@gmail.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc In-Reply-To: <20230411225725.2032862-7-robdclark@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,HK_RANDOM_ENVFROM,HK_RANDOM_FROM, NICE_REPLY_A,RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED 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: X-Mailing-List: linux-kernel@vger.kernel.org On 11/04/2023 23:56, Rob Clark wrote: > From: Rob Clark > > Add support to dump GEM stats to fdinfo. > > v2: Fix typos, change size units to match docs, use div_u64 > v3: Do it in core > > Signed-off-by: Rob Clark > Reviewed-by: Emil Velikov > --- > Documentation/gpu/drm-usage-stats.rst | 21 ++++++++ > drivers/gpu/drm/drm_file.c | 76 +++++++++++++++++++++++++++ > include/drm/drm_file.h | 1 + > include/drm/drm_gem.h | 19 +++++++ > 4 files changed, 117 insertions(+) > > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst > index b46327356e80..b5e7802532ed 100644 > --- a/Documentation/gpu/drm-usage-stats.rst > +++ b/Documentation/gpu/drm-usage-stats.rst > @@ -105,6 +105,27 @@ object belong to this client, in the respective memory region. > Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' > indicating kibi- or mebi-bytes. > > +- drm-shared-memory: [KiB|MiB] > + > +The total size of buffers that are shared with another file (ie. have more > +than a single handle). > + > +- drm-private-memory: [KiB|MiB] > + > +The total size of buffers that are not shared with another file. > + > +- drm-resident-memory: [KiB|MiB] > + > +The total size of buffers that are resident in system memory. I think this naming maybe does not work best with the existing drm-memory- keys. How about introduce the concept of a memory region from the start and use naming similar like we do for engines? drm-memory-$CATEGORY-$REGION: ... Then we document a bunch of categories and their semantics, for instance: 'size' - All reachable objects 'shared' - Subset of 'size' with handle_count > 1 'resident' - Objects with backing store 'active' - Objects in use, subset of resident 'purgeable' - Or inactive? Subset of resident. We keep the same semantics as with process memory accounting (if I got it right) which could be desirable for a simplified mental model. (AMD needs to remind me of their 'drm-memory-...' keys semantics. If we correctly captured this in the first round it should be equivalent to 'resident' above. In any case we can document no category is equal to which category, and at most one of the two must be output.) Region names we at most partially standardize. Like we could say 'system' is to be used where backing store is system RAM and others are driver defined. Then discrete GPUs could emit N sets of key-values, one for each memory region they support. I think this all also works for objects which can be migrated between memory regions. 'Size' accounts them against all regions while for 'resident' they only appear in the region of their current placement, etc. Userspace can aggregate if it wishes to do so but kernel side should not. > + > +- drm-purgeable-memory: [KiB|MiB] > + > +The total size of buffers that are purgeable. > + > +- drm-active-memory: [KiB|MiB] > + > +The total size of buffers that are active on one or more rings. > + > - drm-cycles- > > Engine identifier string must be the same as the one specified in the > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index 37dfaa6be560..46fdd843bb3a 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -42,6 +42,7 @@ > #include > #include > #include > +#include > #include > > #include "drm_crtc_internal.h" > @@ -871,6 +872,79 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e) > } > EXPORT_SYMBOL(drm_send_event); > > +static void print_size(struct drm_printer *p, const char *stat, size_t sz) > +{ > + const char *units[] = {"", " KiB", " MiB"}; > + unsigned u; > + > + for (u = 0; u < ARRAY_SIZE(units) - 1; u++) { > + if (sz < SZ_1K) > + break; > + sz = div_u64(sz, SZ_1K); > + } > + > + drm_printf(p, "%s:\t%zu%s\n", stat, sz, units[u]); > +} > + > +static void print_memory_stats(struct drm_printer *p, struct drm_file *file) > +{ > + struct drm_gem_object *obj; > + struct { > + size_t shared; > + size_t private; > + size_t resident; > + size_t purgeable; > + size_t active; > + } size = {0}; > + bool has_status = false; > + int id; > + > + spin_lock(&file->table_lock); > + idr_for_each_entry (&file->object_idr, obj, id) { > + enum drm_gem_object_status s = 0; > + > + if (obj->funcs && obj->funcs->status) { > + s = obj->funcs->status(obj); > + has_status = true; > + } > + > + if (obj->handle_count > 1) { > + size.shared += obj->size; > + } else { > + size.private += obj->size; > + } > + > + if (s & DRM_GEM_OBJECT_RESIDENT) { > + size.resident += obj->size; > + } else { > + /* If already purged or not yet backed by pages, don't > + * count it as purgeable: > + */ > + s &= ~DRM_GEM_OBJECT_PURGEABLE; Side question - why couldn't resident buffers be purgeable? Did you mean for the if branch check to be active here? But then it wouldn't make sense for a driver to report active _and_ purgeable.. > + } > + > + if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) { > + size.active += obj->size; > + > + /* If still active, don't count as purgeable: */ > + s &= ~DRM_GEM_OBJECT_PURGEABLE; Another side question - I guess this tidies a race in reporting? If so not sure it matters given the stats are all rather approximate. > + } > + > + if (s & DRM_GEM_OBJECT_PURGEABLE) > + size.purgeable += obj->size; > + } One concern I have here is that it is all based on obj->size. That is, there is no provision for drivers to implement page level granularity. So correct reporting in use cases such as VM BIND in the future wouldn't work unless it was a driver hook to get almost all of the info above. At which point common code is just a loop. TBF I don't know if any drivers do sub obj->size backing store granularity today, but I think it is sometimes to be sure of before proceeding. Second concern is what I touched upon in the first reply block - if the common code blindly loops over all objects then on discrete GPUs it seems we get an 'aggregate' value here which is not what I think we want. We rather want to have the ability for drivers to list stats per individual memory region. > + spin_unlock(&file->table_lock); > + > + print_size(p, "drm-shared-memory", size.shared); > + print_size(p, "drm-private-memory", size.private); > + print_size(p, "drm-active-memory", size.active); > + > + if (has_status) { > + print_size(p, "drm-resident-memory", size.resident); > + print_size(p, "drm-purgeable-memory", size.purgeable); > + } > +} > + > /** > * drm_fop_show_fdinfo - helper for drm file fops > * @seq_file: output stream > @@ -904,6 +978,8 @@ void drm_fop_show_fdinfo(struct seq_file *m, struct file *f) > > if (dev->driver->show_fdinfo) > dev->driver->show_fdinfo(&p, file); > + > + print_memory_stats(&p, file); > } > EXPORT_SYMBOL(drm_fop_show_fdinfo); > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index dfa995b787e1..e5b40084538f 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -41,6 +41,7 @@ > struct dma_fence; > struct drm_file; > struct drm_device; > +struct drm_printer; > struct device; > struct file; > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 189fd618ca65..213917bb6b11 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -42,6 +42,14 @@ > struct iosys_map; > struct drm_gem_object; > > +/** > + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting > + */ > +enum drm_gem_object_status { > + DRM_GEM_OBJECT_RESIDENT = BIT(0), > + DRM_GEM_OBJECT_PURGEABLE = BIT(1), > +}; > + > /** > * struct drm_gem_object_funcs - GEM object functions > */ > @@ -174,6 +182,17 @@ struct drm_gem_object_funcs { > */ > int (*evict)(struct drm_gem_object *obj); > > + /** > + * @status: > + * > + * The optional status callback can return additional object state > + * which determines which stats the object is counted against. The > + * callback is called under table_lock. Racing against object status > + * change is "harmless", and the callback can expect to not race > + * against object destruction. > + */ > + enum drm_gem_object_status (*status)(struct drm_gem_object *obj); Does this needs to be in object funcs and couldn't be consolidated to driver level? Regards, Tvrtko > + > /** > * @vm_ops: > *