From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 428D81E47CE for ; Tue, 7 Jan 2025 10:43:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.46 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736246620; cv=none; b=aRSPHXNColO/T9Xd4y5hJ6Q5Xje83OiMHOXzUw3BpXDRSK+JSgfaixFZf16IlBkA/7LWTD7REq1JKcPRocSMn1OpLppTpeNUdip667v0Qc7nnYtO9sSFCqa0YJ3BmNwUr4v+4k6Pb7iceNlh5lGCRT2pVpffZk01Z3eVuDtsbLY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736246620; c=relaxed/simple; bh=FIGb9p1p6fE9kiY3S8qFo+b1+MaA9cvj5iPTFqa0ZrY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=tRINn293tA5psEgonJl40wSdhC+NPvRmNp445suP2CoBLl95IGBU5OmHCdEnPOIxHgQD293kM3fUTsZK+FVVZguNQDQ98p3GItb9nCY2Bb5lrMPbmS5XYhhNQu1MBKSrrE/RbTK0aQ7HP2IuCrxzO0gOWMXjWDmv+kRJ1IUMbUw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net; spf=none smtp.mailfrom=ursulin.net; dkim=pass (2048-bit key) header.d=ursulin-net.20230601.gappssmtp.com header.i=@ursulin-net.20230601.gappssmtp.com header.b=Qn/mAClH; arc=none smtp.client-ip=209.85.128.46 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ursulin.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ursulin-net.20230601.gappssmtp.com header.i=@ursulin-net.20230601.gappssmtp.com header.b="Qn/mAClH" Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-43634b570c1so109938475e9.0 for ; Tue, 07 Jan 2025 02:43:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ursulin-net.20230601.gappssmtp.com; s=20230601; t=1736246615; x=1736851415; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=JBvwPO7uMOawN7K708HQlElZF2GeKij7i1s54/VpKLc=; b=Qn/mAClHcbu+XxNN/8Km8p2N4aCOOwEugh6J6m/xrH28uKCFlHS0LkAuWRZkxFe/Ux NVCXpbyTsIvaSNJSyEk+RBp8tqS1gjzTHXqDZNSpkoWcr+K0GCt5PQxTfoR/MgIklb7k FsZZ0xUceKqm8RJFzS+JxKj+e2VSWAN8+RcOXLxzt0tabdjkOYzTc5lEIc6wW63skbW4 +2EapMuvtzFouI26uo6bs5MTNoGKios33p/zcO2vs6uiIo62ZlXyuekocYMmy4aE9CPE PsQX7WLf8W25O07OSXNR9amVI2OofVTW+bPuWwniEkz6xToQZ91g//h3FLqce02JJV6j H6ZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736246615; x=1736851415; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=JBvwPO7uMOawN7K708HQlElZF2GeKij7i1s54/VpKLc=; b=FBMOP7Dvlt/Q4oqbx/VoB1dTU7zTlDdIM7gTodvW1ZEBkFVOXHu8hWFxr1l0eF8c59 jC5Sq/MNImcBS5cgkyf/SfpWSBqSGjM8UoNV44u2Jw0mKP7dh5tS4ZsqGebGCfP57+vf XZqFe9um/m68qJ/NdnyilHx6tUo2Uw/5zDK4fd4nojOsVtSKo0UJn4M9Z+S5nOLY7GLH vUJ1b/oOiA7ZfqsSJowZc2msnJepkZgxL07nDu8srZJSP8ZjzyZE31wqKSb8C0IyZFcE 8wHU6DFgzsIqlAw4wNvm/l6ftS/INfcAhFd2c6qHMyzoXNKzVgQOLfNetOCt5hP9bhkd vuLg== X-Forwarded-Encrypted: i=1; AJvYcCVbD6n7a+SNkCXnC2InMDH/XhWnqJgObqtuA2I+1arddn1lNP0MBJr3fw79dLwKEBj4M4u5A/cnhORUgQ0=@vger.kernel.org X-Gm-Message-State: AOJu0YyYN/V5ZvuWJWyxFL2FZm5k5lciNLDLcOH7IRyZNVS0qInqYcsN wKLaSOSsaBGWxyrsOFre9RTB85ukzZH44Z8gdGp8ICZh7k0++OXWTcSn9yNP4ZA= X-Gm-Gg: ASbGnctV8RRrDyeO595LZ1zjTLIwQq+DFx2Cm5O7XOaFnJUJyIroCKK7FBW8DqMrTRX uZ55FkHQdVRxXxcp9vqdGQo3Vjm2PHaC0gyLppSHgthUcd4QdwlYs/lf80OJjwAIUR+Kac6Ef0+ 3Ibl9YJmCHfyDFqZZZEIlWuoSb0zyvgrOB9+I5GQDGNFaZ7MXTNvDXJmSQxqaXIdTL10+gXoMq3 mNCZzj78MtX3JPN2DN3ac+p7s2aso+NfXkh1dBag+m7mtTKX5BSLFfUGXOjOHQ5TewLgoYW X-Google-Smtp-Source: AGHT+IFG9ndcnZmaPryeFVAEaT7e8OE+j+/swO6098jqZEuPtknSWTu+gb1vbHX6ZmbX2bo+mdB/tw== X-Received: by 2002:a05:600c:45cf:b0:434:fbd5:2f0a with SMTP id 5b1f17b1804b1-43668642e7bmr539646015e9.9.1736246615171; Tue, 07 Jan 2025 02:43:35 -0800 (PST) Received: from [192.168.0.101] ([90.241.98.187]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43656b1143dsm624441675e9.18.2025.01.07.02.43.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Jan 2025 02:43:34 -0800 (PST) Message-ID: <0f70fa7b-c6fa-4bb5-8f33-c40e9cfaaa80@ursulin.net> Date: Tue, 7 Jan 2025 10:43:34 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/2] Documentation/gpu: Add fdinfo meanings of drm-*-internal memory tags To: =?UTF-8?Q?Adri=C3=A1n_Mart=C3=ADnez_Larumbe?= Cc: Boris Brezillon , Steven Price , Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , kernel@collabora.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Mihail Atanassov References: <20241218181844.886043-1-adrian.larumbe@collabora.com> <20241218181844.886043-3-adrian.larumbe@collabora.com> <1ef1d07b-bfa9-4e52-bfa0-20f569752701@ursulin.net> <2sb72aco2lc5hlvwn7hpc5k27naep7u2s64lc6qzk4ruy6jkhd@c2dfhvhe76yt> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 06/01/2025 16:53, Adrián Martínez Larumbe wrote: > On 03.01.2025 10:49, Tvrtko Ursulin wrote: >> On 02/01/2025 22:18, Adrián Martínez Larumbe wrote: >>> On 02.01.2025 21:59, Tvrtko Ursulin wrote: >>>> >>>> On 18/12/2024 18:18, Adrián Martínez Larumbe wrote: >>>>> From: Adrián Larumbe >>>>> >>>>> A previous commit enabled display of driver-internal kernel BO sizes >>>>> through the device file's fdinfo interface. >>>>> >>>>> Expand the description of the relevant driver-specific key:value pairs >>>>> with the definitions of the new drm-*-internal ones. >>>>> >>>>> Signed-off-by: Adrián Larumbe >>>>> Reviewed-by: Mihail Atanassov >>>>> --- >>>>> Documentation/gpu/panthor.rst | 14 ++++++++++++++ >>>>> 1 file changed, 14 insertions(+) >>>>> >>>>> diff --git a/Documentation/gpu/panthor.rst b/Documentation/gpu/panthor.rst >>>>> index 3f8979fa2b86..23aa3d67c9d2 100644 >>>>> --- a/Documentation/gpu/panthor.rst >>>>> +++ b/Documentation/gpu/panthor.rst >>>>> @@ -26,6 +26,10 @@ the currently possible format options: >>>>> drm-cycles-panthor: 94439687187 >>>>> drm-maxfreq-panthor: 1000000000 Hz >>>>> drm-curfreq-panthor: 1000000000 Hz >>>>> + drm-total-internal: 10396 KiB >>>>> + drm-shared-internal: 0 >>>>> + drm-active-internal: 10396 KiB >>>>> + drm-resident-internal: 10396 KiB >>>>> drm-total-memory: 16480 KiB >>>>> drm-shared-memory: 0 >>>>> drm-active-memory: 16200 KiB >>>>> @@ -44,3 +48,13 @@ driver by writing into the appropriate sysfs node:: >>>>> Where `N` is a bit mask where cycle and timestamp sampling are respectively >>>>> enabled by the first and second bits. >>>>> + >>>>> +Possible `drm-*-internal` keys are: `total`, `active`, `resident` and `shared`. >>>>> +These values convey the sizes of the internal driver-owned shmem BO's that >>>>> +aren't exposed to user-space through a DRM handle, like queue ring buffers, >>>>> +sync object arrays and heap chunks. Because they are all allocated and pinned >>>>> +at creation time, `drm-resident-internal` and `drm-total-internal` should always >>>>> +be equal. `drm-active-internal` shows the size of kernel BO's associated with >>>>> +VM's and groups currently being scheduled for execution by the GPU. >>>>> +`drm-shared-internal` is unused at present, but in the future it might stand for >>>>> +the size of executable FW regions, since they do not belong to an open file context. >>>> >>>> The description is way too specific, too tied to some of the implementations. >>> >>> These are panthor-specific key:value pairs. I was in the belief that drivers >>> could define their own when it suits their interest beyond the DRM-wide ones >>> defined in the drm-fdinfo spec. >>> >>>> I also don't remember that you ever explained why totting up the internal >>>> objects into existing regions isn't good enough. I keep asking, you keep not >>>> explaining. Or I missed your emails somehow. >>> >>> It's not that it's not good enough, but rather that it cannot be done in the >>> current state of affairs. drm_show_memory_stats() defines its own >>> drm_memory_stats struct as an automatic variable so we don't have access to it >>> from anywhere else in the driver. In a previous revision of the patch series I >>> had come up with a workaround that would let drivers pass a function pointer to >>> drm_show_memory_stats() which would gather those numbers in a driver-specific >>> way, but it didn't seem to get any traction. >> >> Side note - i915 and amdgpu manage to do it so it is not that it is not >> possible. >> >>>> And you keep not copying me on the thread. Copying people who expressed >>>> interest, gave past feedback, etc should be the norm. >>> >>> I did not CC you on this series because these are all panthor-specific changes >>> which do not touch on any DRM fdinfo-wide code, and also because I didn't think >>> that driver-specific key:value pairs needed the approval of the drm-fdinfo core >>> maintainers. >> >> Ah my bad.. sorry! I saw drm-internal-* and did not spot it is actually *in* >> panthor.rst. So I think you just need to rename those to panthor- prefix. Same >> as amdgpu has its own private keys amd-evicted-vram etc. > > This complicates things because that means I can no longer use > drm_print_memory_stat(), since print_size() will prefix every single key with Maybe you even shouldn't because it does not seem to fit that well? For instance you define total and resident must always match. And shared is unused. So why expose the duplicate keys to start with? You will not be able to change it later and keep userspace compatibility. And keys like shared "might be used for X in the future" is also not very useful. What does userspace do with those when parsing? It cannot make a decision. > "drm-". But then not using print_size() means I'm giving up on the nice unit > size selection loop, which I guess I could just copy and paste inside Panthor, > but I do remember a recent patch series where the unit selection criteria > changed slightly so wouldn't like to keep these manually sync'd. > > There's also the thing that the units I'm displaying here match up nicely with > those representing the size of DRM objects with a UM-facing handle, so crafting > my own function to display these when the only difference is a single key prefix > seems like an overkill. I guess drm_print_memory_stats() and the functions > underneath should offer more flexibility, but I guess that's something that can > be discussed in a later patch series. What you describe here could be just some refactoring is needed rather than being a huge problem. Like export a new helper from the code which takes the prefix as argument. > And then there's the following statement in Documentation/gpu/drm-usage-stats.rst:24: > > "- All keys shall be prefixed with `drm-`." > > It doesn't say Driver-specific keys should begin with the name of the driver. In > fact, it seems neither AMD nor Intel drivers have theirs documented. > > In light of all this, I'd much rather not modify the names of Panthor-specific > fdinfo's internal memory keys. Amdgpu indeed fails to document its specific keys but some lenience there since it predated the standardisation and a patch can be submitted to fix that. I've been making some changes too to make it use more of common keys and helpers. No other drivers appear to have driver specific keys at this point. Which ones do you see on Intel side? If indeed there are none then that leaves the question of what drm-usage-stats.rst means when it says: - All keys shall be prefixed with `drm-`. We could for instance clarify that applies to common keys and that the driver specific keys should use a different prefix. To me it feels like doing that (clarifying documentation) and tweaking your series to add some core helpers you could use would lead to the better end result. Regards, Tvrtko >>>> Until we can clarify the above points I don't think this can go in. >>>> >>>> Regards, >>>> >>>> Tvrtko >>> >>> Adrian Larumbe > > Adrian Larumbe