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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B8718C636D6 for ; Thu, 23 Feb 2023 15:01:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4F8786B0071; Thu, 23 Feb 2023 10:01:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4A7EA6B0074; Thu, 23 Feb 2023 10:01:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2FA4A6B0075; Thu, 23 Feb 2023 10:01:58 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 1DA116B0071 for ; Thu, 23 Feb 2023 10:01:58 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 9DFC6C0BD5 for ; Thu, 23 Feb 2023 15:01:57 +0000 (UTC) X-FDA: 80498871474.09.48F7FFD Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf24.hostedemail.com (Postfix) with ESMTP id 23C5B180059 for ; Thu, 23 Feb 2023 15:01:52 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=G7IzjT6O; spf=pass (imf24.hostedemail.com: domain of dakr@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=dakr@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677164513; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=k9OY3DOmsttxoPAiFSmCq7gtwaYekEfHfeSsFmsCyrY=; b=ph7fS80b4pZr49d+IyWdSr/13f73MqQx5mZbJkAEQlMCVUCdj3F3pg9FKmwrnuKSknGMkK uf8czxtgM5zDyRir2HFISX8f3lUV8YV3MbWE7LnsMjkFKe1fFg0tVOu2n0VSpbr4mHA0TM sUpFxNW8Pc/VYuofg7V31Qkiyt7XvJA= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=G7IzjT6O; spf=pass (imf24.hostedemail.com: domain of dakr@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=dakr@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677164513; a=rsa-sha256; cv=none; b=199pcYGD2AAO5O7a+rS9qlSyFk68Rug7G2EzMl7cOzW2fpKjNi8I8379+z8YdBinaxlVKz JX5dvwAf6zUq3wD5F0/dskwoy6ySYPEUXL0d9b8df3zDojj6XY4hvjlhuGBPcGrfCPaILO zWYeuO9fC3yCcRkBdgzcWFjnScjHyLY= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677164512; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=k9OY3DOmsttxoPAiFSmCq7gtwaYekEfHfeSsFmsCyrY=; b=G7IzjT6OeIXoNBTGYPC4KKcpeXbwcF2jVuWpO8LofEm8G7wTHQRrfnWZ8leGgbYs5IrE84 KOjNmt6XCRxQMF0xZYZX85T9W59rmbauPNjH8oJaKY1Rpa1kS8tUBLdjKIrAbzpnAEB5jj vMxyfreD36IydTwo/7SJtS4mr235lIc= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-232-xnYruHWkN2WOg0UlFThZqg-1; Thu, 23 Feb 2023 10:01:46 -0500 X-MC-Unique: xnYruHWkN2WOg0UlFThZqg-1 Received: by mail-ed1-f71.google.com with SMTP id h13-20020a0564020e8d00b004a26ef05c34so14845384eda.16 for ; Thu, 23 Feb 2023 07:01:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=k9OY3DOmsttxoPAiFSmCq7gtwaYekEfHfeSsFmsCyrY=; b=0IQewPKN5yTmg5cjaBp8V9i5sR9lJntXzAIgKnsGF6Cdxpw9h0TKkW7dHKYDrr89Fj wYchfMlrIIcSCU73rxmS4PWMLelDMRXSJMQ6jhOZ3sbmR8R5FPc5vEeCGET6J5Wuwc8i MfCZXSbusKvcEsCXC5xuEGbBgHnlzrHWeplZjRDnKWBzcsIJrhZVoEh7kP96EwztQzp8 s02q2y1hKF7avWpGUMMhQ1Lyvz29tI/QOI8e3wIamof/JuEQM7CcKz6keZ7s0Xrhku6O 97DyDjma33o9mjaktgsdWH844H1nEmvEx6eV5ZiwV8lztGO4UQT6V6QcnubiQYe2uKam yB5g== X-Gm-Message-State: AO0yUKV0+AUQHN2WOeX1sCy7qucBUnBraj4Xs19Tan7HtSyyE+C8RHmm L1gPkIw9afl5yRKXdxUSj+WJyoXbTPPbZNQ/22QLFNfMtDNBgOYSyXBjZ5t0Z1TpqqxAfoZVTnp TUsoH26ACZC4= X-Received: by 2002:a17:907:3e22:b0:8e3:8543:8e71 with SMTP id hp34-20020a1709073e2200b008e385438e71mr11141661ejc.40.1677164503830; Thu, 23 Feb 2023 07:01:43 -0800 (PST) X-Google-Smtp-Source: AK7set9x3TEkF4rjKsOMblKg8nB91RIygBy/qyspdCpvJfYp5Ifo24giDUJ/K+JxqZGl9KaboO5b6g== X-Received: by 2002:a17:907:3e22:b0:8e3:8543:8e71 with SMTP id hp34-20020a1709073e2200b008e385438e71mr11141608ejc.40.1677164503488; Thu, 23 Feb 2023 07:01:43 -0800 (PST) Received: from ?IPV6:2a02:810d:4b3f:de78:642:1aff:fe31:a15c? ([2a02:810d:4b3f:de78:642:1aff:fe31:a15c]) by smtp.gmail.com with ESMTPSA id h7-20020a170906260700b008b8ae79a72bsm7650214ejc.135.2023.02.23.07.01.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Feb 2023 07:01:42 -0800 (PST) Message-ID: <734e3c77-a536-95ca-bcae-1e1e86940a48@redhat.com> Date: Thu, 23 Feb 2023 15:12:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings To: =?UTF-8?Q?Christian_K=c3=b6nig?= Cc: airlied@gmail.com, daniel@ffwll.ch, tzimmermann@suse.de, mripard@kernel.org, corbet@lwn.net, bskeggs@redhat.com, Liam.Howlett@oracle.com, matthew.brost@intel.com, boris.brezillon@collabora.com, alexdeucher@gmail.com, ogabbay@kernel.org, bagasdotme@gmail.com, willy@infradead.org, jason@jlekstrand.net, linux-doc@vger.kernel.org, nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-mm@kvack.org, Dave Airlie References: <20230217134422.14116-1-dakr@redhat.com> <20230217134422.14116-6-dakr@redhat.com> <70ba382f-1559-289a-4922-ca9c371aaf59@amd.com> <29ea3705-5634-c204-c1da-d356b6dfbafc@amd.com> <83755119-083d-7d66-fca0-ca306c841d9c@redhat.com> <7780a9b9-d6bd-6f3f-9c31-aafacb09db1f@amd.com> From: Danilo Krummrich Organization: RedHat In-Reply-To: <7780a9b9-d6bd-6f3f-9c31-aafacb09db1f@amd.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: o7g6o387jrbjudy1gqr4zptfnqd11rf9 X-Rspamd-Queue-Id: 23C5B180059 X-HE-Tag: 1677164512-838939 X-HE-Meta: U2FsdGVkX18N57zlMSxk5px1kII0RX60EXvsCNh0cVoJHStxZlYJVEwdIMAhdUdRAPgIIzUBfW+s2P+jLf3d1mqCKzimgHOrWgSxKMmsWt6iATjriLxRpaeI0anVaFCJpbb/JeFQSDpiybIb0cvUGAlFSkkayTUFLTwsX9rwOS0z6q6lLoCetqPct6IgKg9TCASFVcy5VL7XzaKecAP30f4jTwZ5q0KUdgi7yc+zFv2ifpwW7UjyEJzGNNw3Zbwmq/rLxJylfE+fwOI4AytFlkEeH8dmmHDu26a/EaGKvTPFRyx/BEMmC54sHz5sqbs3EFmB6N0J6Mxy+DiD32k45Se7/M793NeDp28qZXD3lKgRcVDrgICVxkYFAJtcSIDndEBbIESH/iv2SnDVInJi+pIqqrVmJo0bNeELVZfW430mg8miRe7tLwWatFzKDwdAZfwy1N00uMZI9DIUMc2Tka3/kueyAnAIXm1z2wg6ZrsjwNo+6v8usFynrHIt67FjNy35MkwBZo5OkCqlyM4b0UnUzgz5f9njB2IeBpn6wJWXm9i6WPQzss/28KimYBb0iJAUNH+TkvRKgf8t+AFDj6UZo7lf8vT1fFUdjzlo89gMZzRcuMC/dAlz1QArpzcA+aSoUiwCRK5USt5TGceDASHSC4PqzilumsK4+9D0jKPG6OOHno97Z8hOvhsepki2dTRF5VKUUL30NX6hso7ZYKrMnoTI4n1C0+P5UZ0km9FoAtQv+c7+OsHrsXItvX2WgMFyS1rqVBDrGqJYMBGUGPkmcBs/6gy5oglfHkwu8PQBLUbpWHCeh9Uust72JioATRlUyULi7ae1WOYG5KF7EQuLJTkOnPeRkLt0Q1iSwr2xwWvFe25o4b4GKOYdTXbjnK2qrE2M4N3GMrYA98cmpV4uZhBde4yukHZb9zK8UwuYv9cVxbAEuAKx+s6yeF46Ed97MbtaG7MNHf6/RKX X5bm7W72 yCy8IALguW2DCSFQZnL/CRa7u5GKh+FaEynVDlvn55eBFOTSXsIwo28a7N/uZE4GRpq68BqN5Hp7L4C1LfmYnV/6vipDiTVgdeSPrxceFIQYcfewON1igI/0luOOkU6Uw3C5qPbzdaJ3X8oQC9FdAvBc/ire0e8/vR7neWRbYUQRgLFtW7LlckGLrH1d6Z2EVIMZUCeiAR25oE6iNXANWANqRCW+orz8wJLdUgpPA/ku2aGOhVVM5zgLamj/fip5dJ+NNmHfOFVKaG/mRQBvL0/AdsXuDKq6FXwzVsalmnmzniNwTRYmTdCrpluC5C3t/0Ize20Uu0sxuGw8xp9IGkuG+WThhW+hVXQR1gzL6t/nnjT7FGGVsqT0kKzpDU6PvqrGOzRJW61G9mG0Qsj/LDb3/4w== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2/23/23 08:06, Christian König wrote: > Am 22.02.23 um 17:40 schrieb Danilo Krummrich: >> On 2/22/23 16:14, Christian König wrote: >>> Am 22.02.23 um 16:07 schrieb Danilo Krummrich: >>>> On 2/22/23 11:25, Christian König wrote: >>>>> Am 17.02.23 um 14:44 schrieb Danilo Krummrich: >>>> >>>> >>>> >>>>>> +/** >>>>>> + * DOC: Overview >>>>>> + * >>>>>> + * The DRM GPU VA Manager, represented by struct >>>>>> drm_gpuva_manager keeps track >>>>>> + * of a GPU's virtual address (VA) space and manages the >>>>>> corresponding virtual >>>>>> + * mappings represented by &drm_gpuva objects. It also keeps >>>>>> track of the >>>>>> + * mapping's backing &drm_gem_object buffers. >>>>>> + * >>>>>> + * &drm_gem_object buffers maintain a list (and a corresponding >>>>>> list lock) of >>>>>> + * &drm_gpuva objects representing all existent GPU VA mappings >>>>>> using this >>>>>> + * &drm_gem_object as backing buffer. >>>>>> + * >>>>>> + * If the &DRM_GPUVA_MANAGER_REGIONS feature is enabled, a GPU VA >>>>>> mapping can >>>>>> + * only be created within a previously allocated >>>>>> &drm_gpuva_region, which >>>>>> + * represents a reserved portion of the GPU VA space. GPU VA >>>>>> mappings are not >>>>>> + * allowed to span over a &drm_gpuva_region's boundary. >>>>>> + * >>>>>> + * GPU VA regions can also be flagged as sparse, which allows >>>>>> drivers to create >>>>>> + * sparse mappings for a whole GPU VA region in order to support >>>>>> Vulkan >>>>>> + * 'Sparse Resources'. >>>>> >>>>> Well since we have now found that there is absolutely no technical >>>>> reason for having those regions could we please drop them? >>>> >>>> I disagree this was the outcome of our previous discussion. >>>> >>>> In nouveau I still need them to track the separate sparse page >>>> tables and, as you confirmed previously, Nvidia cards are not the >>>> only cards supporting this feature. >>>> >>>> The second reason is that with regions we can avoid merging between >>>> buffers, which saves some effort. However, I agree that this >>>> argument by itself probably doesn't hold too much, since you've >>>> pointed out in a previous mail that: >>>> >>>> >>>> 1) If we merge and decide to only do that inside certain boundaries >>>> then those boundaries needs to be provided and checked against. This >>>> burns quite some CPU cycles >>>> >>>> 2) If we just merge what we can we might have extra page table >>>> updates which cost time and could result in undesired side effects. >>>> >>>> 3) If we don't merge at all we have additional housekeeping for the >>>> mappings and maybe hw restrictions. >>>> >>>> >>>> However, if a driver uses regions to track its separate sparse page >>>> tables anyway it gets 1) for free, which is a nice synergy. >>>> >>>> I totally agree that regions aren't for everyone though. Hence, I >>>> made them an optional feature and by default regions are disabled. >>>> In order to use them drm_gpuva_manager_init() must be called with >>>> the DRM_GPUVA_MANAGER_REGIONS feature flag. >>>> >>>> I really would not want to open code regions or have two GPUVA >>>> manager instances in nouveau to track sparse page tables. That would >>>> be really messy, hence I hope we can agree on this to be an optional >>>> feature. >>> >>> I absolutely don't think that this is a good idea then. This separate >>> handling of sparse page tables is completely Nouveau specific. >> >> Actually, I rely on what you said in a previous mail when I say it's, >> potentially, not specific to nouveau. >> >> >> This sounds similar to what AMD hw used to have up until gfx8 (I >> think), basically sparse resources where defined through a separate >> mechanism to the address resolution of the page tables. I won't rule >> out that other hardware has similar approaches. >> > > Ok, sounds like I didn't made my point here clear: AMD does have that > same mechanism for older hw you try to implement here for Nouveau, but > we have *abandoned* it because it is to much trouble and especially > overhead to support! In other words we have said "Ok we would need two > separate components to cleanly handle that, one for newer hw and one for > older hw.". My point was more about the potential existence of other hardware having similar concepts. I, personally, can't judge whether actually making use of having separate sparse page tables (or similar concepts) makes sense for other drivers or not. I think it depends on how the hardware works, which limitations it has in handling page tables, etc. I definitely recognize your experience and that for AMD you decided its not worth using a similar mechanism. I would definitely be interested in the details. Do you mind sharing them? However, I think we need to differentiate between whether for AMD hardware you just found an approach that worked out better for your specific hardware or whether something is fundamentally broken with separate sparse page tables (or similar concepts) in general. Do you think there is something fundamentally broken with such an approach? And if so, why? > > What you now try to do is to write one component which works for both. > We have already exercised this idea and came to the conclusion that it's > not a good path to go down. So you're basically just repeating our mistake. > > I mean if it's just for Nouveau then I would say feel free to do > whatever you want, but since this component is supposed to be used by > more drivers then I strongly think we need to tackle this from a > different side. > >>> Even when it's optional feature mixing this into the common handling >>> is exactly what I pointed out as not properly separating between >>> hardware specific and hardware agnostic functionality. >> >> Optionally having regions is *not* a hardware specific concept, >> drivers might use it for a hardware specific purpose though. Which >> potentially is is the case for almost every DRM helper. >> >> Drivers can use regions only for the sake of not merging between >> buffer boundaries as well. Some drivers might prefer this over "never >> merge" or "always merge", depending on the cost of re-organizing page >> tables for unnecessary splits/merges, without having the need of >> tracking separate sparse page tables. >> >> Its just that I think *if* a driver needs to track separate sparse >> page tables anyways its a nice synergy since then there is no extra >> cost for getting this optimization. > > Well exactly that's the point: I really don't believe that this comes > without extra costs. If you already have to store some information for purpose A and an optional purpose B requires the exact same information you would get B for free. Which other costs would you see here? > > What we could maybe do is to have an two separate functions, one for > updating the data structures and one for merging. When you now call the > merging function with a limit you don't get mappings merged over that > limit and if you don't call the merging function at all you don't get > merges. Having a separate merging function would work. However, I am against an interface that takes limit parameters. Having such an interface signals general compliance with tracking regions to drivers, but without the offer to do this job in a generic way. This sounds like a bad compromise to me. I think we should either accept that some drivers might have a purpose of tracking regions and hence *optionally* support them or have clear evidence that tracking regions never ever make sense at all regardless of how a specific hardware handles it's page tables. Allowing drivers to set the merge strategy, however, is a good idea. I could also just add corresponding feature flags to let the driver pick. > > But we should have definitely not have the tracking of the ranges inside > the common component. This is something separated. > >>> This is exactly the problem we ran into with TTM as well and I've >>> spend a massive amount of time to clean that up again. > >> >> Admittedly, I don't know what problems you are referring to. However, >> I don't see which kind of trouble it could cause by allowing drivers >> to track regions optionally. > > Take a look at my 2020 presentation about TTM on FOSDEM. > > Regards, > Christian. > >> >>> Regards, >>> Christian. >>> >>>> >>>>> >>>>> I don't really see a need for them any more. >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>> >>> >> >