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:   Fri, 23 Jun 2023 02:24:52 +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>,
        <airlied@gmail.com>, <daniel@ffwll.ch>, <tzimmermann@suse.de>,
        <mripard@kernel.org>, <corbet@lwn.net>, <bskeggs@redhat.com>,
        <Liam.Howlett@oracle.com>, <boris.brezillon@collabora.com>,
        <alexdeucher@gmail.com>, <ogabbay@kernel.org>,
        <bagasdotme@gmail.com>, <willy@infradead.org>,
        <jason@jlekstrand.net>, <dri-devel@lists.freedesktop.org>,
        <nouveau@lists.freedesktop.org>, <linux-doc@vger.kernel.org>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Donald Robson <donald.robson@imgtec.com>,
        Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA
 mappings
Message-ID: <ZJUCdP1T+arh3M+H@DUT025-TGLU.fm.intel.com>
References: <20230620004217.4700-1-dakr@redhat.com>
 <20230620004217.4700-4-dakr@redhat.com>
 <cf6846ea-5bd0-0b41-b7e6-901c70701751@amd.com>
 <a8edf75b-e0f7-a6c7-7d29-f0d39923549b@redhat.com>
 <41aecd10-9956-0752-2838-34c97834f0c8@amd.com>
 <bcde7ea3-fbab-3a18-e810-64b6589ddb18@redhat.com>
 <86ef9898-c4b6-f4c0-7ad3-3ffe5358365a@amd.com>
 <c1f05169-dec0-22ee-52fa-c8070678394e@redhat.com>
 <2f502150-c1f8-615c-66d9-c3fb59b8c409@redhat.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2f502150-c1f8-615c-66d9-c3fb59b8c409@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?Gm/86BGZ+YTcbnFMab24YDu+sqHkGIcmQwdhNTMektEM1F0AOOplq3NKrt?=
 =?iso-8859-1?Q?e9S+8s6d4KaqsAiH4efxEjDcNO7srw7hUSL55/IDRB9iusLPfK7O3vkrzg?=
 =?iso-8859-1?Q?UjMGTX7Fve0P5Iozf+fQP290v4mlIw8lVHDANg7xrXPyi8UUwPnyIB/fyk?=
 =?iso-8859-1?Q?M+zHzQuQWvZFSVNeMq36TT1yvsjYppfNBCNxasMpG3q2GDDARyqL/te6lY?=
 =?iso-8859-1?Q?c04yaaWVT9IE5NAG/Zds66d9vmf9sfYx5SZA0MwI/MslmKXsJfbyA7ZQ8D?=
 =?iso-8859-1?Q?PsvOjxeS5RGilKK5YBnlVezjGB8yKKH0svDxYOFO6IxWQuWU7VjhvbUcYO?=
 =?iso-8859-1?Q?nhJUvYzm+g3cZlWihm05AVxjCgDb7ppsqnFeeCj3lDT12kMv9rxMoHq4wU?=
 =?iso-8859-1?Q?mH74fTZ8K6EUvZvGurPdMVwT1SfnGnvXfTHf5j1KWRojptDbRgOQh1jLov?=
 =?iso-8859-1?Q?A4Wyd0coljeVWKb8lQ00xytNwYB6omOXeSP1q39pEZeu2kgvcPuoQpPE7G?=
 =?iso-8859-1?Q?8RpL2vu/gvoc9Mu16WanmUZANuzWd1rOfeFTumMKbdq1q5JtRrAEnRzDUt?=
 =?iso-8859-1?Q?IJbP76t5Jr1MghRvYHRAqOtbNZOGxbwonhWhAhxfJgLIisSMZ+cl7w+nuA?=
 =?iso-8859-1?Q?zAOuS8bLktzSQe4kmsTOuW2qunaxlevw8INIJLusSHGrhXAeuK6meLYYpi?=
 =?iso-8859-1?Q?SPZryK7IhyTxwX1DQfDpY1dkcZYfisVfaZ5qNW+zFT1tK/gH60N16e0In6?=
 =?iso-8859-1?Q?QEOnoGA/VMZrSbeIh3z7hf15djEPJsYHRY+F5mUn90Lp2GgP/HDE1X1W09?=
 =?iso-8859-1?Q?+uo53gq5FtEAaLgQpYx6kt6HCFVysz+JBGnOnNYdyjQAdcYs96foy49GoK?=
 =?iso-8859-1?Q?zYO6GnvhNm9t2thBwnpi20Z3adHBBQhkZlcY0SRNaM2HdBHRx4o9PQuE/E?=
 =?iso-8859-1?Q?2OeWiAN+H+Q8iTwpCNYYBATzu6VJCBUaYHKP1VgYrMFyt9khZeMRJ7oukL?=
 =?iso-8859-1?Q?lFlsR4wiNVM3DH6mISTcDjJ4vddlUxYiAtKSgzByJ3g3bVRn9B2gzoimMl?=
 =?iso-8859-1?Q?BIu22/mRDf9G7XNy+JUAVLZz/zZoF2GpKzTFdPIqw7dVeuSRAjkzxXs51S?=
 =?iso-8859-1?Q?sqDkJJOXwo2YKhWHcwu3GpWf+9SgcunGumAIV6HpNZumz1IfFyjiMgdj3i?=
 =?iso-8859-1?Q?fXEDpENPWENyJL96vv4kAMG0kj/BpWyGUzji+F8mEsrJq5n5ticcq8aCR5?=
 =?iso-8859-1?Q?YQsHRE7IC3MCnUMPilxe81NDs5KAmY3y78XobCnRVhXR+Y1m1z2sPQjTCP?=
 =?iso-8859-1?Q?LMZkfjUEmdk0MEML61SZyn1dadfcD5SXZ3sU3d4Kwk6Z7fRoj4dooagFUU?=
 =?iso-8859-1?Q?pNyndCf+IbHfIwjgRCc/7n7nKsaw8Ppnj4lEbo/Q8wVYRsYPhW1NM6WQdp?=
 =?iso-8859-1?Q?fsA7Iln2aE0vueqwmIgGEFDfWujwmPCydmhtJE4Nfryt5G80FpJzYe7gic?=
 =?iso-8859-1?Q?f3KaN1QVal3uHcRzrHyJsnU3cuLP7efT8PHInQW2Tc60+0/S/shALZ4b2E?=
 =?iso-8859-1?Q?bGz7do223O5dT37UTXm+d38x9AVzNqfbcyKy9gFLAOJYtIJUvC66jQTrdF?=
 =?iso-8859-1?Q?q0fWJg5vPwCK2xIgXjFm011ZG0CQSi/mXcM1DjaxZhQ4tVl1WvxumKxA?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35f61b01-b616-4f9f-a269-08db73911ae1
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB6522.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2023 02:25:26.4433
 (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: b3wezvewqWsCJpjzqAoP/meS/Lb6IWwm8C0Tdhpyew54+D04E1iX7uDP99/LH3bzfYPYe0uCyOmMlw4KXkJMwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4575
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jun 22, 2023 at 05:07:11PM +0200, Danilo Krummrich wrote:
> On 6/22/23 17:04, Danilo Krummrich wrote:
> > On 6/22/23 16:42, Christian K?nig wrote:
> > > Am 22.06.23 um 16:22 schrieb Danilo Krummrich:
> > > > On 6/22/23 15:54, Christian K?nig wrote:
> > > > > Am 20.06.23 um 14:23 schrieb Danilo Krummrich:
> > > > > > Hi Christian,
> > > > > > 
> > > > > > On 6/20/23 08:45, Christian K?nig wrote:
> > > > > > > Hi Danilo,
> > > > > > > 
> > > > > > > sorry for the delayed reply. I've trying to dig
> > > > > > > myself out of a hole at the moment.
> > > > > > 
> > > > > > No worries, thank you for taking a look anyway!
> > > > > > 
> > > > > > > 
> > > > > > > Am 20.06.23 um 02:42 schrieb Danilo Krummrich:
> > > > > > > > [SNIP]
> > > > > > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > > > > > > index bbc721870c13..5ec8148a30ee 100644
> > > > > > > > --- a/include/drm/drm_gem.h
> > > > > > > > +++ b/include/drm/drm_gem.h
> > > > > > > > @@ -36,6 +36,8 @@
> > > > > > > > ? #include <linux/kref.h>
> > > > > > > > ? #include <linux/dma-resv.h>
> > > > > > > > +#include <linux/list.h>
> > > > > > > > +#include <linux/mutex.h>
> > > > > > > > ? #include <drm/drm_vma_manager.h>
> > > > > > > > @@ -379,6 +381,18 @@ struct drm_gem_object {
> > > > > > > > ?????? */
> > > > > > > > ????? struct dma_resv _resv;
> > > > > > > > +??? /**
> > > > > > > > +???? * @gpuva:
> > > > > > > > +???? *
> > > > > > > > +???? * Provides the list of GPU VAs attached to this GEM object.
> > > > > > > > +???? *
> > > > > > > > +???? * Drivers should lock list accesses with
> > > > > > > > the GEMs &dma_resv lock
> > > > > > > > +???? * (&drm_gem_object.resv).
> > > > > > > > +???? */
> > > > > > > > +??? struct {
> > > > > > > > +??????? struct list_head list;
> > > > > > > > +??? } gpuva;
> > > > > > > > +
> > > > > > > > ????? /**
> > > > > > > > ?????? * @funcs:
> > > > > > > > ?????? *
> > > > > > > 
> > > > > > > I'm pretty sure that it's not a good idea to attach
> > > > > > > this directly to the GEM object.
> > > > > > 
> > > > > > Why do you think so? IMHO having a common way to connect
> > > > > > mappings to their backing buffers is a good thing, since
> > > > > > every driver needs this connection anyway.
> > > > > > 
> > > > > > E.g. when a BO gets evicted, drivers can just iterate
> > > > > > the list of mappings and, as the circumstances require,
> > > > > > invalidate the corresponding mappings or to unmap all
> > > > > > existing mappings of a given buffer.
> > > > > > 
> > > > > > What would be the advantage to let every driver
> > > > > > implement a driver specific way of keeping this
> > > > > > connection?
> > > > > 
> > > > > Flexibility. For example on amdgpu the mappings of a BO are
> > > > > groups by VM address spaces.
> > > > > 
> > > > > E.g. the BO points to multiple bo_vm structures which in
> > > > > turn have lists of their mappings.
> > > > 
> > > > Isn't this (almost) the same relationship I introduce with the
> > > > GPUVA manager?
> > > > 
> > > > If you would switch over to the GPUVA manager right now, it
> > > > would be that every GEM has a list of it's mappings (the gpuva
> > > > list). The mapping is represented by struct drm_gpuva (of course
> > > > embedded in driver specific structure(s)) which has a pointer to
> > > > the VM address space it is part of, namely the GPUVA manager
> > > > instance. And the GPUVA manager keeps a maple tree of it's
> > > > mappings as well.
> > > > 
> > > > If you still would like to *directly* (indirectly you already
> > > > have that relationship) keep a list of GPUVA managers (VM
> > > > address spaces) per GEM, you could still do that in a driver
> > > > specific way.
> > > > 
> > > > Do I miss something?
> > > 
> > > How do you efficiently find only the mappings of a BO in one VM?
> > 
> > Actually, I think this case should even be more efficient than with a BO
> > having a list of GPUVAs (or mappings):
> 
> *than with a BO having a list of VMs:
> 
> > 
> > Having a list of GPUVAs per GEM, each GPUVA has a pointer to it's VM.
> > Hence, you'd only need to iterate the list of mappings for a given BO
> > and check the mappings VM pointer.
> > 
> > Having a list of VMs per BO, you'd have to iterate the whole VM to find
> > the mappings having a pointer to the given BO, right?
> > 
> > I'd think that a single VM potentially has more mapping entries than a
> > single BO was mapped in multiple VMs.
> > 
> > Another case to consider is the case I originally had in mind choosing
> > this relationship: finding all mappings for a given BO, which I guess
> > all drivers need to do in order to invalidate mappings on BO eviction.
> > 
> > Having a list of VMs per BO, wouldn't you need to iterate all of the VMs
> > entirely?
> > 

FWIW I agree with Danilo here on basically all points.

- Having GPUVA list per GEM makes a ton of sense wrt eviction and
  invalidation. Xe had this list prior to GPUVA, after GPUVA it is just
  in a common place.
- From a GPUVA to you can resolve a GEM
- GPUVA can have NULL GEM (userptr or sparse binding in Xe)
- From a VM all GPUVA can be resolved via the maple tree

This seems to do everything any reasonable driver would need.

> > > 
> > > Keep in mind that we have cases where one BO is shared with hundreds
> > > of different VMs as well as potentially the number of mappings can
> > > be >10k.
> > > 

Christian - Can you explain this use case? It seems like an odd one but
regardless the GPUVA design still works we'd just have 10k GPUVA in the
BO's list.

> > > > 
> > > > > 
> > > > > Additional to that there is a state maschine associated with
> > > > > the mappings, e.g. if the page tables are up to date or need
> > > > > to be updated etc....
> > > > > 
> > > > > > Do you see cases where this kind of connection between
> > > > > > mappings and backing buffers wouldn't be good enough? If
> > > > > > so, which cases do you have in mind? Maybe we can cover
> > > > > > them in a common way as well?
> > > > > 
> > > > > Yeah, we have tons of cases like that. But I have no idea
> > > > > how to generalize them.
> > > > 
> > > > They could still remain to be driver specific then, right?
> > > 
> > > Well does the mapping has a back pointer to the BO? And can that be
> > > optional NULL if there is no BO?
> > 
> > Yes to both.
> >

In Xe we have GPUVA with NULL BOs for userptrs and for sparse bindings.
It seems to work rather well.

Matt

> > - Danilo
> > 
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > As you wrote in the commit message it's highly
> > > > > > > driver specific what to map and where to map it.
> > > > > > 
> > > > > > In the end the common case should be that in a VA space
> > > > > > at least every mapping being backed by a BO is
> > > > > > represented by a struct drm_gpuva.
> > > > > 
> > > > > Oh, no! We also have mappings not backed by a BO at all! For
> > > > > example for partial resident textures or data routing to
> > > > > internal hw etc...
> > > > > 
> > > > > You can't be sure that a mapping is backed by a BO at all.
> > > > 
> > > > I fully agree, that's why I wrote "the common case should be
> > > > that in a VA space at least every mapping *being backed by a BO*
> > > > is represented by a struct drm_gpuva".
> > > > 
> > > > Mappings not being backed by an actual BO would not be linked to
> > > > a GEM of course.
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Instead I suggest to have a separate structure for
> > > > > > > mappings in a VA space which driver can then add to
> > > > > > > their GEM objects or whatever they want to map into
> > > > > > > their VMs.
> > > > > > 
> > > > > > Which kind of separate structure for mappings? Another
> > > > > > one analogous to struct drm_gpuva?
> > > > > 
> > > > > Well similar to what amdgpu uses BO -> one structure for
> > > > > each combination of BO and VM -> mappings inside that VM
> > > > 
> > > > As explained above, I think that's exactly what the GPUVA
> > > > manager does, just in another order:
> > > > 
> > > > BO has list of mappings, mappings have pointer to VM, VM has
> > > > list (or actually a maple tree) of mappings.
> > > > 
> > > > You see any advantages or disadvantages of either order of
> > > > relationships? For me it looks like it doesn't really matter
> > > > which one to pick.
> > > > 
> > > > - Danilo
> > > > 
> > > > > 
> > > > > Regards,
> > > > > Christian.
> > > > > 
> > > > > > 
> > > > > > - Danilo
> > > > > > 
> > > > > > > 
> > > > > > > Regards,
> > > > > > > Christian.
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
>