Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
> > > > > > > > #include
> > > > > > > > +#include
> > > > > > > > +#include
> > > > > > > > #include
> > > > > > > > @@ -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 _resv lock
> > > > > > > > + * (_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
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
> > As pointed out by Christian, this would optimize the "get all mappings > > backed by a specific BO from a given VM" use case. > > > > The question for me is, do other drivers than amdgpu commonly need this? > > I have no idea. > > > > > And what does amdgpu need this for? Maybe amdgpu does something we're > > not doing (yet)? > > Basically when we do a CS we need to make sure that the VM used by this > CS is up to date. For this we walk over the relocation list of BOs and > check the status of each BO+VM structure. > > This is done because we don't want to update all VMs at the same time, > but rather just those who needs the update. This seems like a legacy from GL and possibly older vulkan, going forward vulkan can't rely on passing lists of objects into the kernel due to things like buffer_device_address, I'm not sure we should optimise for this situation, and moving the tracking list into the drivers is going to mean having a bunch of drivers all having the same boilerplate, to do the same thing just so amdgpu can't avoid doing it. Now there might be some benchmark somewhere we can produce a benefit in this, and if there is then we should consider going this way for all drivers and not just allowing drivers to choose their own path here. > > > > Christian - I know you didn't ask for "do it the way amdgpu does", > > instead you voted for keeping it entirely driver specific. But I think > > everyone is pretty close and I'm still optimistic that we could just > > generalize this. > > Well, you should *not* necessarily do it like amdgpu does! Basically the > implementation in amdgpu was driven by requirements, e.g. we need that, > let's do it like this. > > It's perfectly possible that other requirements (e.g. focus on Vulkan) > lead to a completely different implementation. > > It's just that ideally I would like to have an implementation where I > can apply at least the basics to amdgpu as well. > I think we can still do that just either have an option to disable using the list internally in the gpuva or have the driver keep it's own tracking alongside, there may still be use cases where it can use the gpuva tracking instead of it's own. I don't think we should forklift what is pretty likely to be common code across every driver that uses this going forward just to benefit an amdgpu design decision for older stacks. Dave.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Am 23.06.23 um 15:55 schrieb Danilo Krummrich:
[SNIP]
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.
Yeah, and that is extremely time consuming if you have tons of
mappings in different VMs.
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?
No, you don't seem to understand what I'm suggesting.
Currently you have a list of mappings attached to the BO, so when you
need to make sure that a specific BO is up to date in a specific VM
you either need to iterate over the VM or the BO. Neither of that is
a good idea.
What you need is a representation of the data used for each BO+VM
combination. In other words another indirection which allows you to
handle all the mappings of a BO inside a VM at once.
Ok, after having a quick look at amdgpu, I can see what you mean.
The missing piece for me was that the BO+VM abstraction itself keeps a
list of mappings for this specific BO and VM.
Just to make it obvious for other people following the discussion, let
me quickly sketch up how this approach would look like for the GPUVA
manager:
1. We would need a new structure to represent the BO+VM combination,
something like:
struct drm_gpuva_mgr_gem {
struct drm_gpuva_manager *mgr;
struct drm_gem_object *obj;
struct list_head gpuva_list;
};
with a less horrible name, hopefully.
2. Create an instance of struct drm_gpuva_mgr_gem once a GEM becomes
associated with a GPUVA manager (VM) and attach it to the GEMs, as by
now, "gpuva" list.
In amdgpu, for example, this seems to be the case once a GEM object is
opened, since there is one VM per file_priv.
However, for other drivers this could be different, hence drivers
would need to take care about this.
Yes, exactly that.
3. Attach GPUVAs to the new gpuva_list of the corresponding instance of
struct drm_gpuva_mgr_gem.
4. Drivers would need to clean up the instance of struct
drm_gpuva_mgr_gem, once the GEM is not associated with the GPUVA
manager anymore.
As pointed out by Christian, this would optimize the "get all mappings
backed by a specific BO from a given VM" use case.
The question for me is, do other drivers than amdgpu commonly need this?
I have no idea.
And what does amdgpu need this for? Maybe amdgpu does something we're
not doing (yet)?
Basically when we do a CS we need to make sure that the VM used by this
CS is up to date. For this we walk over the relocation list of BOs and
check the status of each BO+VM structure.
This is done because we don't want to update all VMs at the same time,
but rather just those who needs the update.
Christian - I know you didn't ask for "do it the way amdgpu does",
instead you voted for keeping it entirely driver specific. But I think
everyone is pretty close and I'm still optimistic that we could just
generalize this.
Well, you should *not* necessarily do it like amdgpu does! Basically the
implementation in amdgpu was driven by requirements, e.g. we need that,
let's do it like this.
It's perfectly possible that other requirements (e.g. focus on Vulkan)
lead to a completely different implementation.
It's just that ideally I would like to have an implementation where I
can apply at least the basics to amdgpu as well.
Regards,
Christian.
- Danilo
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?
No, see how amdgpu works.
Regards,
Christian.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
On 6/23/23 09:16, Christian König wrote:
Am 22.06.23 um 17:07 schrieb Danilo Krummrich:
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
#include
+#include
+#include
#include
@@ -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
_resv lock
+ * (_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.
Yeah, and that is extremely time consuming if you have tons of mappings
in different VMs.
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?
No, you don't seem to understand what I'm suggesting.
Currently you have a list of mappings attached to the BO, so when you
need to make sure that a specific BO is up to date in a specific VM you
either need to iterate over the VM or the BO. Neither of that is a good
idea.
What you need is a representation of the data used for each BO+VM
combination. In other words another indirection which allows you to
handle all the mappings of a BO inside a VM at once.
Ok, after having a quick look at amdgpu, I can see what you mean.
The missing piece for me was that the BO+VM abstraction itself keeps a
list of mappings for this specific BO and VM.
Just to make it obvious for other people following the discussion, let
me quickly sketch up how this approach would look like for the GPUVA
manager:
1. We would need a new structure to represent the BO+VM combination,
something like:
struct drm_gpuva_mgr_gem {
struct drm_gpuva_manager *mgr;
struct drm_gem_object *obj;
struct list_head gpuva_list;
};
with a less horrible name, hopefully.
2. Create an instance of struct drm_gpuva_mgr_gem once a GEM becomes
associated with a GPUVA manager (VM) and attach it to the GEMs, as by
now, "gpuva" list.
In amdgpu, for example, this seems to be the case once a GEM object is
opened, since there is one VM per file_priv.
However, for other drivers this could be different, hence drivers would
need to take care about this.
3. Attach GPUVAs to the new gpuva_list of the corresponding instance of
struct drm_gpuva_mgr_gem.
4. Drivers would need to clean up the instance of struct
drm_gpuva_mgr_gem, once the GEM is not associated
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Am 22.06.23 um 17:07 schrieb Danilo Krummrich:
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
#include
+#include
+#include
#include
@@ -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
_resv lock
+ * (_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.
Yeah, and that is extremely time consuming if you have tons of mappings
in different VMs.
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?
No, you don't seem to understand what I'm suggesting.
Currently you have a list of mappings attached to the BO, so when you
need to make sure that a specific BO is up to date in a specific VM you
either need to iterate over the VM or the BO. Neither of that is a good
idea.
What you need is a representation of the data used for each BO+VM
combination. In other words another indirection which allows you to
handle all the mappings of a BO inside a VM at once.
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?
No, see how amdgpu works.
Regards,
Christian.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv
lock
+ * (_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?
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.
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.
- 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
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv
lock
+ * (_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):
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?
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.
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.
- 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
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv
lock
+ * (_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?
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.
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?
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.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv lock
+ * (_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?
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?
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.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv lock
+ * (_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.
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.
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.
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
Regards,
Christian.
- Danilo
Regards,
Christian.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Hi Danilo, One comment below, but otherwise it looks great. Thanks for adding the example! Thanks, Donald On Tue, 2023-06-20 at 02:42 +0200, Danilo Krummrich wrote: > > +/** > + * 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 _gpuva objects. It also keeps track of the > + * mapping's backing _gem_object buffers. > + * > + * _gem_object buffers maintain a list of _gpuva objects representing > + * all existent GPU VA mappings using this _gem_object as backing buffer. > + * > + * GPU VAs can be flagged as sparse, such that drivers may use GPU VAs to > also > + * keep track of sparse PTEs in order to support Vulkan 'Sparse Resources'. > + * > + * The GPU VA manager internally uses a _tree to manage the > + * _gpuva mappings within a GPU's virtual address space. > + * > + * The _gpuva_manager contains a special _gpuva representing the > + * portion of VA space reserved by the kernel. This node is initialized > together > + * with the GPU VA manager instance and removed when the GPU VA manager is > + * destroyed. > + * > + * In a typical application drivers would embed struct drm_gpuva_manager and > + * struct drm_gpuva within their own driver specific structures, there won't > be > + * any memory allocations of it's own nor memory allocations of _gpuva > + * entries. > + * > + * However, the _gpuva_manager needs to allocate nodes for it's internal > + * tree structures when _gpuva entries are inserted. In order to support > + * inserting _gpuva entries from dma-fence signalling critical sections > the > + * _gpuva_manager provides struct drm_gpuva_prealloc. Drivers may create > + * pre-allocated nodes which drm_gpuva_prealloc_create() and subsequently > insert > + * a new _gpuva entry with drm_gpuva_insert_prealloc(). I think it might be worth moving or repeating this paragraph to 'Split and Merge' where I've added the other comment below. I think these functions are only used to set up for drm_gpuva_sm_map(). Please ignore me if I'm wrong. > + */ > + > +/** > + * DOC: Split and Merge > + * > + * Besides it's capability to manage and represent a GPU VA space, the > + * _gpuva_manager also provides functions to let the _gpuva_manager > + * calculate a sequence of operations to satisfy a given map or unmap > request. > + * > + * Therefore the DRM GPU VA manager provides an algorithm implementing > splitting > + * and merging of existent GPU VA mappings with the ones that are requested > to > + * be mapped or unmapped. This feature is required by the Vulkan API to > + * implement Vulkan 'Sparse Memory Bindings' - drivers UAPIs often refer to > this > + * as VM BIND. > + * > + * Drivers can call drm_gpuva_sm_map() to receive a sequence of callbacks > + * containing map, unmap and remap operations for a given newly requested > + * mapping. The sequence of callbacks represents the set of operations to > + * execute in order to integrate the new mapping cleanly into the current > state > + * of the GPU VA space. Here > + * > + * Depending on how the new GPU VA mapping intersects with the existent > mappings > + * of the GPU VA space the _gpuva_fn_ops callbacks contain an arbitrary > + * amount of unmap operations, a maximum of two remap operations and a single > + * map operation. The caller might receive no callback at all if no > operation is > + * required, e.g. if the requested mapping already exists in the exact same > way. >
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
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
#include
+#include
+#include
#include
@@ -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 _resv lock
+ * (_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? 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?
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.
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?
- Danilo
Regards,
Christian.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Hi Danilo,
sorry for the delayed reply. I've trying to dig myself out of a hole at
the moment.
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
#include
+#include
+#include
#include
@@ -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 _resv lock
+* (_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.
As you wrote in the commit message it's highly driver specific what to
map and where to map it.
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.
Regards,
Christian.
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Why are none of these EXPORT_SYMBOL_GPL as it's very linux-internal stuff?
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Hi Danilo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dcb0775d36de28992f56455ab3967b30d380]
url:
https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230620-084448
base: dcb0775d36de28992f56455ab3967b30d380
patch link:https://lore.kernel.org/r/20230620004217.4700-4-dakr%40redhat.com
patch subject: [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA
mappings
config: hexagon-randconfig-r041-20230620
(https://download.01.org/0day-ci/archive/20230620/202306201123.4nvlb3cq-...@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git
4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce:
(https://download.01.org/0day-ci/archive/20230620/202306201123.4nvlb3cq-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202306201123.4nvlb3cq-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_gpuva_mgr.c:676:7: warning: format specifies type
>> 'unsigned long' but the argument has type 'unsigned int' [-Wformat]
676 | return WARN(check_add_overflow(addr, range, ),
|~~~
677 | "GPUVA address limited to %lu bytes, see
Documentation.\n",
|
~~~
| %u
678 | MTREE_INDEX_SIZE);
| ^
drivers/gpu/drm/drm_gpuva_mgr.c:663:26: note: expanded from macro
'MTREE_INDEX_SIZE'
663 | #define MTREE_INDEX_SIZE sizeof(MTREE_INDEX_TYPE)
| ^
include/asm-generic/bug.h:133:29: note: expanded from macro 'WARN'
133 | __WARN_printf(TAINT_WARN, format);
\
| ~~^~~
include/asm-generic/bug.h:97:48: note: expanded from macro '__WARN_printf'
97 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);
\
| ^~~
drivers/gpu/drm/drm_gpuva_mgr.c:1314:25: warning: variable 'prev' set but
not used [-Wunused-but-set-variable]
1314 | struct drm_gpuva *va, *prev = NULL;
|^
2 warnings generated.
vim +676 drivers/gpu/drm/drm_gpuva_mgr.c
668
669 static inline bool
670 drm_gpuva_check_overflow(u64 addr, u64 range)
671 {
672 MTREE_INDEX_TYPE end;
673
674 return WARN(check_add_overflow(addr, range, ),
675 "GPUVA address limited to %lu bytes, see
Documentation.\n",
> 676 MTREE_INDEX_SIZE);
677 }
678
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Hi Danilo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dcb0775d36de28992f56455ab3967b30d380]
url:
https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230620-084448
base: dcb0775d36de28992f56455ab3967b30d380
patch link:https://lore.kernel.org/r/20230620004217.4700-4-dakr%40redhat.com
patch subject: [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA
mappings
config: m68k-allyesconfig
(https://download.01.org/0day-ci/archive/20230620/202306201034.gucldv3r-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce:
(https://download.01.org/0day-ci/archive/20230620/202306201034.gucldv3r-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202306201034.gucldv3r-...@intel.com/
All warnings (new ones prefixed by >>):
In file included from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:56,
from include/linux/kref.h:16,
from include/drm/drm_gem.h:37,
from drivers/gpu/drm/drm_gpuva_mgr.c:28:
drivers/gpu/drm/drm_gpuva_mgr.c: In function 'drm_gpuva_check_overflow':
>> drivers/gpu/drm/drm_gpuva_mgr.c:675:21: warning: format '%lu' expects
>> argument of type 'long unsigned int', but argument 5 has type 'unsigned int'
>> [-Wformat=]
675 | "GPUVA address limited to %lu bytes, see
Documentation.\n",
|
^~
include/asm-generic/bug.h:97:62: note: in definition of macro '__WARN_printf'
97 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);
\
| ^~~
drivers/gpu/drm/drm_gpuva_mgr.c:674:16: note: in expansion of macro 'WARN'
674 | return WARN(check_add_overflow(addr, range, ),
|^~~~
drivers/gpu/drm/drm_gpuva_mgr.c:675:49: note: format string is defined here
675 | "GPUVA address limited to %lu bytes, see
Documentation.\n",
| ~~^
| |
| long unsigned int
| %u
drivers/gpu/drm/drm_gpuva_mgr.c: In function '__drm_gpuva_sm_map':
drivers/gpu/drm/drm_gpuva_mgr.c:1314:32: warning: variable 'prev' set but
not used [-Wunused-but-set-variable]
1314 | struct drm_gpuva *va, *prev = NULL;
|^~~~
vim +675 drivers/gpu/drm/drm_gpuva_mgr.c
668
669 static inline bool
670 drm_gpuva_check_overflow(u64 addr, u64 range)
671 {
672 MTREE_INDEX_TYPE end;
673
674 return WARN(check_add_overflow(addr, range, ),
> 675 "GPUVA address limited to %lu bytes, see
> Documentation.\n",
676 MTREE_INDEX_SIZE);
677 }
678
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Add infrastructure to keep track of GPU virtual address (VA) mappings with a decicated VA space manager implementation. New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers start implementing, allow userspace applications to request multiple and arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is intended to serve the following purposes in this context. 1) Provide infrastructure to track GPU VA allocations and mappings, making use of the maple_tree. 2) Generically connect GPU VA mappings to their backing buffers, in particular DRM GEM objects. 3) Provide a common implementation to perform more complex mapping operations on the GPU VA space. In particular splitting and merging of GPU VA mappings, e.g. for intersecting mapping requests or partial unmap requests. Tested-by: Donald Robson Suggested-by: Dave Airlie Signed-off-by: Danilo Krummrich --- Documentation/gpu/drm-mm.rst| 42 + drivers/gpu/drm/Makefile|1 + drivers/gpu/drm/drm_gem.c |3 + drivers/gpu/drm/drm_gpuva_mgr.c | 1971 +++ include/drm/drm_drv.h |6 + include/drm/drm_gem.h | 52 + include/drm/drm_gpuva_mgr.h | 682 +++ 7 files changed, 2757 insertions(+) create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c create mode 100644 include/drm/drm_gpuva_mgr.h diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index a52e6f4117d6..0a9d54e723a8 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -466,6 +466,48 @@ DRM MM Range Allocator Function References .. kernel-doc:: drivers/gpu/drm/drm_mm.c :export: +DRM GPU VA Manager +== + +Overview + + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :doc: Overview + +Split and Merge +--- + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :doc: Split and Merge + +Locking +--- + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :doc: Locking + +Examples + + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :doc: Examples + +Quirks +-- + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :doc: Quirks + +DRM GPU VA Manager Function References +-- + +.. kernel-doc:: include/drm/drm_gpuva_mgr.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c + :export: + DRM Buddy Allocator === diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 414855e2a463..6d6c9dec66e8 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -45,6 +45,7 @@ drm-y := \ drm_vblank.o \ drm_vblank_work.o \ drm_vma_manager.o \ + drm_gpuva_mgr.o \ drm_writeback.o drm-$(CONFIG_DRM_LEGACY) += \ drm_agpsupport.o \ diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 1a5a2cd0d4ec..cd878ebddbd0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -164,6 +164,9 @@ void drm_gem_private_object_init(struct drm_device *dev, if (!obj->resv) obj->resv = >_resv; + if (drm_core_check_feature(dev, DRIVER_GEM_GPUVA)) + drm_gem_gpuva_init(obj); + drm_vma_node_reset(>vma_node); INIT_LIST_HEAD(>lru_node); } diff --git a/drivers/gpu/drm/drm_gpuva_mgr.c b/drivers/gpu/drm/drm_gpuva_mgr.c new file mode 100644 index ..66989db49cae --- /dev/null +++ b/drivers/gpu/drm/drm_gpuva_mgr.c @@ -0,0 +1,1971 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2022 Red Hat. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + * Authors: + * Danilo Krummrich + * + */ + +#include +#include + +/** + * 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
