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;
Date:   Thu, 19 Jan 2023 05:23:33 +0000
From:   Matthew Brost <matthew.brost@intel.com>
To:     Danilo Krummrich <dakr@redhat.com>
CC:     Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        Dave Airlie <airlied@gmail.com>,
        Alex Deucher <alexdeucher@gmail.com>, <jason@jlekstrand.net>,
        <linux-doc@vger.kernel.org>, <nouveau@lists.freedesktop.org>,
        <corbet@lwn.net>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <bskeggs@redhat.com>,
        <tzimmermann@suse.de>, <airlied@redhat.com>
Subject: Re: [PATCH drm-next 00/14] [RFC] DRM GPUVA Manager & Nouveau VM_BIND
 UAPI
Message-ID: <Y8jT1TazLddqZjG4@DUT025-TGLU.fm.intel.com>
References: <db4fa0fc-c9a6-9a48-c45f-1d655b30aff9@amd.com>
 <02b0bcb8-f69f-93cf-1f56-ec883cb33965@redhat.com>
 <3602500f-05f5-10b8-5ec6-0a6246e2bb6b@amd.com>
 <bcbef353-f579-4e90-1c77-be36bbe61c0f@redhat.com>
 <CADnq5_PGaXFW-z3gt+R+W+vBVdeuL4wMuMOQh4muxU13Bemy3A@mail.gmail.com>
 <0f2d6e1a-a3b5-f323-a29d-caade427292c@redhat.com>
 <CADnq5_Nh-1esiHzvTG+qFBCfMjy21efX-YN2jfGG=WC+-4LwLQ@mail.gmail.com>
 <CAPM=9txMZO1uYj+kVdTfmCwV2Fq8uu_b3i4eq4xhqPEPKBW8Eg@mail.gmail.com>
 <7839c47e-6692-b93b-69a8-9584193cb07d@amd.com>
 <6566870d-6256-8eef-5879-cb13711e4bed@redhat.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6566870d-6256-8eef-5879-cb13711e4bed@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?3uoF7PU8gfYroAJNjJPROFLxw1n/NUHYn04pM2w1/zN+tYK42lwJ0zwILQ?=
 =?iso-8859-1?Q?xC4a5Ahte0ezRl/5swZTBqTupBWTAq52ccf52UiOGdnzSYzus1pIgKC0YY?=
 =?iso-8859-1?Q?tRCreweNPd8eFO1gxrpym0CHvkz47HrGGvji7F9MM37jtLYQRFNloiUJpO?=
 =?iso-8859-1?Q?Z5vPWf3cqL3ShPQfHJhPXVBExw0wERMh8vWOikA5CBzgEsEUxpHD3Crtk5?=
 =?iso-8859-1?Q?5hVMILjQyf90SRYKzshcJVX3UefUzgXr0WmobjjLF7Pn18eNRqUKSW5BZ2?=
 =?iso-8859-1?Q?qEOIaeF95iiZMhD24U+1CMFawkngQGQ5S44O2Fs5sSOKwNGlS2G+XMX3do?=
 =?iso-8859-1?Q?g/an1dMZ4vuq4qb33wEjxt/U5X+7w1hILveOeivbV9K4qsEghEvrII90eP?=
 =?iso-8859-1?Q?bHulWYOQtX2b0brqYMug6HaAqan6TyyopDQ60a0Mp1N5dFXJwASTRg2bW6?=
 =?iso-8859-1?Q?6O+yqvZkdDEEPZFMU5ZVFkG57MCAusomRKVBaj0GE2dqHGtqNDSK7YhY5S?=
 =?iso-8859-1?Q?J48vw1xq+OYE3MMVc6ZY6Ng/Xk3IC8L3+uD1p/4fcd4lU+Hfm3cvdmySpj?=
 =?iso-8859-1?Q?XFITMpzFRS4NKsot0f5HmerGd/Jm2dSNBv904Sy4h2Sq21Lo5S9WIZtAO/?=
 =?iso-8859-1?Q?uvGNgZwSuvc4zEWZcWFzv9fCogmaT96QoBiTk6E4mS88sl2NHojzByEt/x?=
 =?iso-8859-1?Q?EJHTGjqCAMlbrpcXMOBrR+TwDrTN4YLRKW3wvghZouF8cSUNbK2LZRZjQW?=
 =?iso-8859-1?Q?MTYDjy/g5Wku5NucTGh6hVkwhozVoktvEEZDvpS+QeWZ9ol91MK/28BxMg?=
 =?iso-8859-1?Q?CQSZrxaSJHjRPg1zB8Di8hMuItRl8hVaXs2221PUEcZZUtHCXQkJYHdatg?=
 =?iso-8859-1?Q?cjLXwHkKvgRIVZfJ0Z0GxiVZVeVN+x7hKFbg4ccUeChrcvhTXYY12LOBND?=
 =?iso-8859-1?Q?aAn28dNQpaBnuR6ds6L+1WZ+xLjG9KQ1/Vt1vey+hZKxQgMLm/PyuJ/RMo?=
 =?iso-8859-1?Q?2trTyx2nkWHn04cUD9G787yhh4FjDZz4DcbbbhLtj0rLNRda/5coyHqiTU?=
 =?iso-8859-1?Q?WXjV4JvePbJlur9PXpmNagcC22XjboR1nZ3I4q8JADv5dotdrkP+axF7G5?=
 =?iso-8859-1?Q?1BS0Bcnjs51eie2yalrE8zqFBoKJ+2V5fu/WXMbUSdzHpCPCMZ20mGwig2?=
 =?iso-8859-1?Q?V5U+4KtYX26dnW68mH6rnzit5eM95JSI9e1XjLDZI7Go+b9+oaara/dPh1?=
 =?iso-8859-1?Q?x6C2V3OJxMZHKVlwO1Fhz8YrYRB1QwWx2UTm5FfsDl7baH3/dpjuuLjze0?=
 =?iso-8859-1?Q?W38fQkawDNiR3VgT/fxNNsHcU7CziIDmu3rK7144uczwRznCMlLYvuxA2q?=
 =?iso-8859-1?Q?/aBODMRsMyhoJ8Eh+sPruS14M5/nn7IHaAk8bmQzXCKZp8alq/ZnpjQ5AF?=
 =?iso-8859-1?Q?/icznDCTxV0VsVdhai/UE4Qyiqh0Wb2RKv1VKycqb6cKv0uY7op57rjirp?=
 =?iso-8859-1?Q?OioHIdKBWhRJr7PmTE1EiLq8tJdQGUCSE2EZc5xL5WKF1SDAs2JVYZ7Fut?=
 =?iso-8859-1?Q?hTQNpxV9tPTb2P5+sWcCmmnkWtMa1weC2E0tl/EuA9qz5dJWTirCkjqWUx?=
 =?iso-8859-1?Q?9JGsRESt19CPEQPKA2mdnkiGsAYg1KoYhbQguyOHGw2nq4YoTVSPsJSQ?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7a200bb3-e5f4-4ecb-72fa-08daf9dd5800
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB6522.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 05:23:48.7734
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PsLyreD5Emx0SZYLc7xaw1WtGRqO6HDMS5h9Qc+7BhpB8lve9R3YKYOfebuaoGLDse6SzyazvUxtt/qYDRoOWQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7262
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jan 19, 2023 at 05:04:32AM +0100, Danilo Krummrich wrote:
> On 1/18/23 20:48, Christian K?nig wrote:
> > Am 18.01.23 um 20:17 schrieb Dave Airlie:
> > > On Thu, 19 Jan 2023 at 02:54, Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > On Wed, Jan 18, 2023 at 11:50 AM Danilo Krummrich
> > > > <dakr@redhat.com> wrote:
> > > > > 
> > > > > 
> > > > > On 1/18/23 17:30, Alex Deucher wrote:
> > > > > > On Wed, Jan 18, 2023 at 11:19 AM Danilo Krummrich
> > > > > > <dakr@redhat.com> wrote:
> > > > > > > On 1/18/23 16:37, Christian K?nig wrote:
> > > > > > > > Am 18.01.23 um 16:34 schrieb Danilo Krummrich:
> > > > > > > > > Hi Christian,
> > > > > > > > > 
> > > > > > > > > On 1/18/23 09:53, Christian K?nig wrote:
> > > > > > > > > > Am 18.01.23 um 07:12 schrieb Danilo Krummrich:
> > > > > > > > > > > This patch series provides a new UAPI for the Nouveau driver in
> > > > > > > > > > > order to
> > > > > > > > > > > support Vulkan features, such as
> > > > > > > > > > > sparse bindings and sparse
> > > > > > > > > > > residency.
> > > > > > > > > > > 
> > > > > > > > > > > Furthermore, with the DRM GPUVA
> > > > > > > > > > > manager it provides a new DRM core
> > > > > > > > > > > feature to
> > > > > > > > > > > keep track of GPU virtual address
> > > > > > > > > > > (VA) mappings in a more generic way.
> > > > > > > > > > > 
> > > > > > > > > > > The DRM GPUVA manager is indented to help drivers implement
> > > > > > > > > > > userspace-manageable
> > > > > > > > > > > GPU VA spaces in reference to the Vulkan API. In order to achieve
> > > > > > > > > > > this goal it
> > > > > > > > > > > serves the following purposes in this context.
> > > > > > > > > > > 
> > > > > > > > > > > ?????? 1) Provide a dedicated range allocator to track GPU VA
> > > > > > > > > > > allocations and
> > > > > > > > > > > ????????? mappings, making use of the drm_mm range allocator.
> > > > > > > > > > This means that the ranges are allocated
> > > > > > > > > > by the kernel? If yes that's
> > > > > > > > > > a really really bad idea.
> > > > > > > > > No, it's just for keeping track of the
> > > > > > > > > ranges userspace has allocated.
> > > > > > > > Ok, that makes more sense.
> > > > > > > > 
> > > > > > > > So basically you have an IOCTL which asks kernel
> > > > > > > > for a free range? Or
> > > > > > > > what exactly is the drm_mm used for here?
> > > > > > > Not even that, userspace provides both the base
> > > > > > > address and the range,
> > > > > > > the kernel really just keeps track of things.
> > > > > > > Though, writing a UAPI on
> > > > > > > top of the GPUVA manager asking for a free range instead would be
> > > > > > > possible by just adding the corresponding wrapper functions to get a
> > > > > > > free hole.
> > > > > > > 
> > > > > > > Currently, and that's what I think I read out of
> > > > > > > your question, the main
> > > > > > > benefit of using drm_mm over simply stuffing the
> > > > > > > entries into a list or
> > > > > > > something boils down to easier collision detection and iterating
> > > > > > > sub-ranges of the whole VA space.
> > > > > > Why not just do this in userspace?? We have a range manager in
> > > > > > libdrm_amdgpu that you could lift out into libdrm or some other
> > > > > > helper.
> > > > > The kernel still needs to keep track of the mappings within the various
> > > > > VA spaces, e.g. it silently needs to unmap mappings that are backed by
> > > > > BOs that get evicted and remap them once they're validated (or swapped
> > > > > back in).
> > > > Ok, you are just using this for maintaining the GPU VM space in
> > > > the kernel.
> > > > 
> > > Yes the idea behind having common code wrapping drm_mm for this is to
> > > allow us to make the rules consistent across drivers.
> > > 
> > > Userspace (generally Vulkan, some compute) has interfaces that pretty
> > > much dictate a lot of how VMA tracking works, esp around lifetimes,
> > > sparse mappings and splitting/merging underlying page tables, I'd
> > > really like this to be more consistent across drivers, because already
> > > I think we've seen with freedreno some divergence from amdgpu and we
> > > also have i915/xe to deal with. I'd like to at least have one place
> > > that we can say this is how it should work, since this is something
> > > that *should* be consistent across drivers mostly, as it is more about
> > > how the uapi is exposed.
> > 
> > That's a really good idea, but the implementation with drm_mm won't work
> > like that.
> > 
> > We have Vulkan applications which use the sparse feature to create
> > literally millions of mappings. That's why I have fine tuned the mapping

Is this not an application issue? Millions of mappings seems a bit
absurd to me.

> > structure in amdgpu down to ~80 bytes IIRC and save every CPU cycle
> > possible in the handling of that.

We might need to bit of work here in Xe as our xe_vma structure is quite
big as we currently use it as dumping ground for various features.

> 
> That's a valuable information. Can you recommend such an application for
> testing / benchmarking?
>

Also interested.
 
> Your optimization effort sounds great. May it be worth thinking about
> generalizing your approach by itself and stacking the drm_gpuva_manager on
> top of it?
>

FWIW the Xe is on board with the drm_gpuva_manager effort, we basically
open code all of this right now. I'd like to port over to
drm_gpuva_manager ASAP so we can contribute and help find a viable
solution for all of us.

Matt
 
> > 
> > A drm_mm_node is more in the range of ~200 bytes and certainly not
> > suitable for this kind of job.
> > 
> > I strongly suggest to rather use a good bunch of the amdgpu VM code as
> > blueprint for the common infrastructure.
> 
> I will definitely have look.
> 
> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > Dave.
> > 
>