Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
On 18/03/2024 15:05, Christian König wrote:
Am 18.03.24 um 15:24 schrieb Maíra Canal:
Not that the CC list wasn't big enough, but I'm adding MM folks
in the CC list.
On 3/18/24 11:04, Christian König wrote:
Am 18.03.24 um 14:28 schrieb Maíra Canal:
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags
that better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init()
that allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to
shmem_file_setup().
One strategy for reducing churn, and so the number of
drivers this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(),
and make drm_gem_object_init() call that one with a NULL
argument.
I would even go a step further into the other direction.
The shmem backed GEM object is just some special handling
as far as I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at
least would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not
satisying as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm
missing the connection why a different mount point helps with
using huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for
a DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory
pressure makes the thp code split the trailing huge page. I
haven't checked if that still applies.
Other than that I don't know if some drivers/platforms could
have problems if they have some limitations or hardcoded
assumptions when they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I
can see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not
have that handling in each individual driver, but rather
centralized in the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge
page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously
allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory
pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to
create much extra memory pressure for this use case.
As far as I can see background for the option is that files in
tmpfs usually have a varying size, so it usually isn't beneficial
to allocate a huge page just to find that the shmem file is much
smaller than what's needed.
But GEM objects have a fixed size. So we of hand knew if we need
4KiB or 1GiB and can therefore directly allocate huge pages if they
are available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure
is high?
Well as driver developers/maintainers we simply don't do that. This
is the job of the shmem code.
For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO
allocation (and initially, I tried that), I would end up with hangs
in some applications.
Yeah, that is what I
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 18.03.24 um 15:24 schrieb Maíra Canal:
Not that the CC list wasn't big enough, but I'm adding MM folks
in the CC list.
On 3/18/24 11:04, Christian König wrote:
Am 18.03.24 um 14:28 schrieb Maíra Canal:
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags
that better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init()
that allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to
shmem_file_setup().
One strategy for reducing churn, and so the number of
drivers this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(),
and make drm_gem_object_init() call that one with a NULL
argument.
I would even go a step further into the other direction.
The shmem backed GEM object is just some special handling
as far as I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at
least would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not
satisying as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm
missing the connection why a different mount point helps with
using huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for
a DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory
pressure makes the thp code split the trailing huge page. I
haven't checked if that still applies.
Other than that I don't know if some drivers/platforms could
have problems if they have some limitations or hardcoded
assumptions when they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I
can see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not
have that handling in each individual driver, but rather
centralized in the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge
page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously
allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory
pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to
create much extra memory pressure for this use case.
As far as I can see background for the option is that files in
tmpfs usually have a varying size, so it usually isn't beneficial
to allocate a huge page just to find that the shmem file is much
smaller than what's needed.
But GEM objects have a fixed size. So we of hand knew if we need
4KiB or 1GiB and can therefore directly allocate huge pages if they
are available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure
is high?
Well as driver developers/maintainers we simply don't do that. This
is the job of the shmem code.
For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO
allocation (and initially, I tried that), I would end up with hangs
in some applications.
Yeah, that is what I meant with the trivial optimisation to the
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Not that the CC list wasn't big enough, but I'm adding MM folks
in the CC list.
On 3/18/24 11:04, Christian König wrote:
Am 18.03.24 um 14:28 schrieb Maíra Canal:
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init()
that allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to
shmem_file_setup().
One strategy for reducing churn, and so the number of
drivers this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and
make drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far
as I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not
satisying as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing
the connection why a different mount point helps with using
huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a
DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory
pressure makes the thp code split the trailing huge page. I
haven't checked if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions
when they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can
see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have
that handling in each individual driver, but rather centralized in
the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to
create much extra memory pressure for this use case.
As far as I can see background for the option is that files in tmpfs
usually have a varying size, so it usually isn't beneficial to
allocate a huge page just to find that the shmem file is much smaller
than what's needed.
But GEM objects have a fixed size. So we of hand knew if we need 4KiB
or 1GiB and can therefore directly allocate huge pages if they are
available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure is
high?
Well as driver developers/maintainers we simply don't do that. This is
the job of the shmem code.
For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO allocation
(and initially, I tried that), I would end up with hangs in some
applications.
Yeah, that is what I meant with the trivial optimisation to the shmem
code. Essentially when you have 2MiB+4KiB as
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 18.03.24 um 14:28 schrieb Maíra Canal:
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init()
that allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to
shmem_file_setup().
One strategy for reducing churn, and so the number of
drivers this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and
make drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far
as I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not
satisying as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing
the connection why a different mount point helps with using
huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a
DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory
pressure makes the thp code split the trailing huge page. I
haven't checked if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions
when they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can
see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have
that handling in each individual driver, but rather centralized in
the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to
create much extra memory pressure for this use case.
As far as I can see background for the option is that files in tmpfs
usually have a varying size, so it usually isn't beneficial to
allocate a huge page just to find that the shmem file is much smaller
than what's needed.
But GEM objects have a fixed size. So we of hand knew if we need 4KiB
or 1GiB and can therefore directly allocate huge pages if they are
available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure is
high?
Well as driver developers/maintainers we simply don't do that. This is
the job of the shmem code.
For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO allocation
(and initially, I tried that), I would end up with hangs in some
applications.
Yeah, that is what I meant with the trivial optimisation to the shmem
code. Essentially when you have 2MiB+4KiB as BO size then the shmem code
should use a 2MiB and a 4KiB page to back them, but what it currently
does is to use two 2MiB
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
On 3/18/24 10:28, Maíra Canal wrote:
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers
this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and
make drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far
as I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not
satisying as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing
the connection why a different mount point helps with using huge
pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a
DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory
pressure makes the thp code split the trailing huge page. I haven't
checked if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions
when they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can
see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have
that handling in each individual driver, but rather centralized in
the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to create
much extra memory pressure for this use case.
As far as I can see background for the option is that files in tmpfs
usually have a varying size, so it usually isn't beneficial to
allocate a huge page just to find that the shmem file is much smaller
than what's needed.
But GEM objects have a fixed size. So we of hand knew if we need 4KiB
or 1GiB and can therefore directly allocate huge pages if they are
available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure is
high? For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO allocation
(and initially, I tried that), I would end up with hangs in some
applications.
At least, for V3D, I wouldn't like to see THP being used for all the
allocations. But, we have maintainers of other drivers in the CC.
Okay, I'm thinking about a compromise. What if we create a gemfs
mountpoint in the DRM core and everytime we init a object, we can
choose if we will use huge pages or not. Therefore,
drm_gem_shmem_create()
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Hi Christian,
On 3/18/24 10:10, Christian König wrote:
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers
this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and
make drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far as
I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not satisying
as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing
the connection why a different mount point helps with using huge
pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a
DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory pressure
makes the thp code split the trailing huge page. I haven't checked
if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions when
they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can
see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have
that handling in each individual driver, but rather centralized in
the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to create
much extra memory pressure for this use case.
As far as I can see background for the option is that files in tmpfs
usually have a varying size, so it usually isn't beneficial to allocate
a huge page just to find that the shmem file is much smaller than what's
needed.
But GEM objects have a fixed size. So we of hand knew if we need 4KiB or
1GiB and can therefore directly allocate huge pages if they are
available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
The matter is: how do we define the point where the memory pressure is
high? For example, notice that in this implementation of Super Pages
for the V3D driver, I only use a Super Page if the BO is bigger than
2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM
available for the GPU. If I created huge pages for every BO allocation
(and initially, I tried that), I would end up with hangs in some
applications.
At least, for V3D, I wouldn't like to see THP being used for all the
allocations. But, we have maintainers of other drivers in the CC.
Best Regards,
- Maíra
So THP is almost always beneficial for GEM even if the driver doesn't
actually need it. The only potential case I can think of which might not
be handled gracefully is the tail pages, e.g. huge + 4kib.
But that is
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 18.03.24 um 13:42 schrieb Maíra Canal:
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might
want to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers
this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and
make drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far as
I can see.
So I would rather suggest to rename all drm_gem_* function
which only deal with the shmem backed GEM object into
drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not satisying
as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing
the connection why a different mount point helps with using huge
pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is
'never', see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a
DRM device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"),
back then the understanding was within_size may overallocate,
meaning there would be some space wastage, until the memory pressure
makes the thp code split the trailing huge page. I haven't checked
if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions when
they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can
see this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have
that handling in each individual driver, but rather centralized in
the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Well that's the point I'm disagreeing with. THP doesn't seem to create
much extra memory pressure for this use case.
As far as I can see background for the option is that files in tmpfs
usually have a varying size, so it usually isn't beneficial to allocate
a huge page just to find that the shmem file is much smaller than what's
needed.
But GEM objects have a fixed size. So we of hand knew if we need 4KiB or
1GiB and can therefore directly allocate huge pages if they are
available and object large enough to back them with.
If the memory pressure is so high that we don't have huge pages
available the shmem code falls back to standard pages anyway.
So THP is almost always beneficial for GEM even if the driver doesn't
actually need it. The only potential case I can think of which might not
be handled gracefully is the tail pages, e.g. huge + 4kib.
But that is trivial to optimize in the shmem code when the final size of
the file is known beforehand.
Regards,
Christian.
Best Regards,
- Maíra
Regards,
Christian.
Te Cc is plenty large so perhaps someone else will have additional
information. :)
Regards,
Tvrtko
I mean it would make this patch here even smaller.
Regards,
Christian.
Regards,
Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Hi Christian,
On 3/12/24 10:48, Christian König wrote:
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might want
to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers
this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and make
drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far as I
can see.
So I would rather suggest to rename all drm_gem_* function which
only deal with the shmem backed GEM object into drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not satisying
as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing the
connection why a different mount point helps with using huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is 'never',
see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a DRM
device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), back
then the understanding was within_size may overallocate, meaning there
would be some space wastage, until the memory pressure makes the thp
code split the trailing huge page. I haven't checked if that still
applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions when
they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can see
this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have that
handling in each individual driver, but rather centralized in the DRM code.
I don't see a point in enabling THP for all shmem drivers. A huge page
is only useful if the driver is going to use it. On V3D, for example,
I only need huge pages because I need the memory contiguously allocated
to implement Super Pages. Otherwise, if we don't have the Super Pages
support implemented in the driver, I would be creating memory pressure
without any performance gain.
Best Regards,
- Maíra
Regards,
Christian.
Te Cc is plenty large so perhaps someone else will have additional
information. :)
Regards,
Tvrtko
I mean it would make this patch here even smaller.
Regards,
Christian.
Regards,
Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might want
to have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be
created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers
this patch touches, could be to add a lower level
drm_gem_object_init() (which takes vfsmount, call it
__drm_gem_object_init(), or drm__gem_object_init_mnt(), and make
drm_gem_object_init() call that one with a NULL argument.
I would even go a step further into the other direction. The
shmem backed GEM object is just some special handling as far as I
can see.
So I would rather suggest to rename all drm_gem_* function which
only deal with the shmem backed GEM object into drm_gem_shmem_*.
That makes sense although it would be very churny. I at least
would be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say
driver wants to use huge pages for performance? Or not satisying
as you question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing the
connection why a different mount point helps with using huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance
passing huge=within_size or huge=always option. Default is 'never',
see man 5 tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a DRM
device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), back
then the understanding was within_size may overallocate, meaning there
would be some space wastage, until the memory pressure makes the thp
code split the trailing huge page. I haven't checked if that still
applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions when
they iterate the sg list.
Yeah, that was the whole point behind my question. As far as I can see
this isn't driver specific, but platform specific.
I might be wrong here, but I think we should then probably not have that
handling in each individual driver, but rather centralized in the DRM code.
Regards,
Christian.
Te Cc is plenty large so perhaps someone else will have additional
information. :)
Regards,
Tvrtko
I mean it would make this patch here even smaller.
Regards,
Christian.
Regards,
Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
On 12/03/2024 10:37, Christian König wrote:
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
On 12/03/2024 10:23, Christian König wrote:
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
On 12/03/2024 08:59, Christian König wrote:
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might want to
have a
different mountpoint, for which we pass in mount flags that
better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that
allow us to
define the tmpfs mountpoint where the GEM object will be created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers this
patch touches, could be to add a lower level drm_gem_object_init()
(which takes vfsmount, call it __drm_gem_object_init(), or
drm__gem_object_init_mnt(), and make drm_gem_object_init() call
that one with a NULL argument.
I would even go a step further into the other direction. The shmem
backed GEM object is just some special handling as far as I can see.
So I would rather suggest to rename all drm_gem_* function which
only deal with the shmem backed GEM object into drm_gem_shmem_*.
That makes sense although it would be very churny. I at least would
be on the fence regarding the cost vs benefit.
Yeah, it should clearly not be part of this patch here.
Also the explanation why a different mount point helps with
something isn't very satisfying.
Not satisfying as you think it is not detailed enough to say driver
wants to use huge pages for performance? Or not satisying as you
question why huge pages would help?
That huge pages are beneficial is clear to me, but I'm missing the
connection why a different mount point helps with using huge pages.
Ah right, same as in i915, one needs to mount a tmpfs instance passing
huge=within_size or huge=always option. Default is 'never', see man 5
tmpfs.
Thanks for the explanation, I wasn't aware of that.
Mhm, shouldn't we always use huge pages? Is there a reason for a DRM
device to not use huge pages with the shmem backend?
AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), back
then the understanding was within_size may overallocate, meaning there
would be some space wastage, until the memory pressure makes the thp
code split the trailing huge page. I haven't checked if that still applies.
Other than that I don't know if some drivers/platforms could have
problems if they have some limitations or hardcoded assumptions when
they iterate the sg list.
Te Cc is plenty large so perhaps someone else will have additional
information. :)
Regards,
Tvrtko
I mean it would make this patch here even smaller.
Regards,
Christian.
Regards,
Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin: On 12/03/2024 10:23, Christian König wrote: Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin: On 12/03/2024 08:59, Christian König wrote: Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin: Hi Maira, On 11/03/2024 10:05, Maíra Canal wrote: For some applications, such as using huge pages, we might want to have a different mountpoint, for which we pass in mount flags that better match our usecase. Therefore, add a new parameter to drm_gem_object_init() that allow us to define the tmpfs mountpoint where the GEM object will be created. If this parameter is NULL, then we fallback to shmem_file_setup(). One strategy for reducing churn, and so the number of drivers this patch touches, could be to add a lower level drm_gem_object_init() (which takes vfsmount, call it __drm_gem_object_init(), or drm__gem_object_init_mnt(), and make drm_gem_object_init() call that one with a NULL argument. I would even go a step further into the other direction. The shmem backed GEM object is just some special handling as far as I can see. So I would rather suggest to rename all drm_gem_* function which only deal with the shmem backed GEM object into drm_gem_shmem_*. That makes sense although it would be very churny. I at least would be on the fence regarding the cost vs benefit. Yeah, it should clearly not be part of this patch here. Also the explanation why a different mount point helps with something isn't very satisfying. Not satisfying as you think it is not detailed enough to say driver wants to use huge pages for performance? Or not satisying as you question why huge pages would help? That huge pages are beneficial is clear to me, but I'm missing the connection why a different mount point helps with using huge pages. Ah right, same as in i915, one needs to mount a tmpfs instance passing huge=within_size or huge=always option. Default is 'never', see man 5 tmpfs. Thanks for the explanation, I wasn't aware of that. Mhm, shouldn't we always use huge pages? Is there a reason for a DRM device to not use huge pages with the shmem backend? I mean it would make this patch here even smaller. Regards, Christian. Regards, Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
On 12/03/2024 10:23, Christian König wrote: Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin: On 12/03/2024 08:59, Christian König wrote: Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin: Hi Maira, On 11/03/2024 10:05, Maíra Canal wrote: For some applications, such as using huge pages, we might want to have a different mountpoint, for which we pass in mount flags that better match our usecase. Therefore, add a new parameter to drm_gem_object_init() that allow us to define the tmpfs mountpoint where the GEM object will be created. If this parameter is NULL, then we fallback to shmem_file_setup(). One strategy for reducing churn, and so the number of drivers this patch touches, could be to add a lower level drm_gem_object_init() (which takes vfsmount, call it __drm_gem_object_init(), or drm__gem_object_init_mnt(), and make drm_gem_object_init() call that one with a NULL argument. I would even go a step further into the other direction. The shmem backed GEM object is just some special handling as far as I can see. So I would rather suggest to rename all drm_gem_* function which only deal with the shmem backed GEM object into drm_gem_shmem_*. That makes sense although it would be very churny. I at least would be on the fence regarding the cost vs benefit. Yeah, it should clearly not be part of this patch here. Also the explanation why a different mount point helps with something isn't very satisfying. Not satisfying as you think it is not detailed enough to say driver wants to use huge pages for performance? Or not satisying as you question why huge pages would help? That huge pages are beneficial is clear to me, but I'm missing the connection why a different mount point helps with using huge pages. Ah right, same as in i915, one needs to mount a tmpfs instance passing huge=within_size or huge=always option. Default is 'never', see man 5 tmpfs. Regards, Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin: On 12/03/2024 08:59, Christian König wrote: Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin: Hi Maira, On 11/03/2024 10:05, Maíra Canal wrote: For some applications, such as using huge pages, we might want to have a different mountpoint, for which we pass in mount flags that better match our usecase. Therefore, add a new parameter to drm_gem_object_init() that allow us to define the tmpfs mountpoint where the GEM object will be created. If this parameter is NULL, then we fallback to shmem_file_setup(). One strategy for reducing churn, and so the number of drivers this patch touches, could be to add a lower level drm_gem_object_init() (which takes vfsmount, call it __drm_gem_object_init(), or drm__gem_object_init_mnt(), and make drm_gem_object_init() call that one with a NULL argument. I would even go a step further into the other direction. The shmem backed GEM object is just some special handling as far as I can see. So I would rather suggest to rename all drm_gem_* function which only deal with the shmem backed GEM object into drm_gem_shmem_*. That makes sense although it would be very churny. I at least would be on the fence regarding the cost vs benefit. Yeah, it should clearly not be part of this patch here. Also the explanation why a different mount point helps with something isn't very satisfying. Not satisfying as you think it is not detailed enough to say driver wants to use huge pages for performance? Or not satisying as you question why huge pages would help? That huge pages are beneficial is clear to me, but I'm missing the connection why a different mount point helps with using huge pages. Regards, Christian. Regards, Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
On 12/03/2024 08:59, Christian König wrote: Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin: Hi Maira, On 11/03/2024 10:05, Maíra Canal wrote: For some applications, such as using huge pages, we might want to have a different mountpoint, for which we pass in mount flags that better match our usecase. Therefore, add a new parameter to drm_gem_object_init() that allow us to define the tmpfs mountpoint where the GEM object will be created. If this parameter is NULL, then we fallback to shmem_file_setup(). One strategy for reducing churn, and so the number of drivers this patch touches, could be to add a lower level drm_gem_object_init() (which takes vfsmount, call it __drm_gem_object_init(), or drm__gem_object_init_mnt(), and make drm_gem_object_init() call that one with a NULL argument. I would even go a step further into the other direction. The shmem backed GEM object is just some special handling as far as I can see. So I would rather suggest to rename all drm_gem_* function which only deal with the shmem backed GEM object into drm_gem_shmem_*. That makes sense although it would be very churny. I at least would be on the fence regarding the cost vs benefit. Also the explanation why a different mount point helps with something isn't very satisfying. Not satisfying as you think it is not detailed enough to say driver wants to use huge pages for performance? Or not satisying as you question why huge pages would help? Regards, Tvrtko
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might want to have a
different mountpoint, for which we pass in mount flags that better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that allow us to
define the tmpfs mountpoint where the GEM object will be created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers this
patch touches, could be to add a lower level drm_gem_object_init()
(which takes vfsmount, call it __drm_gem_object_init(), or
drm__gem_object_init_mnt(), and make drm_gem_object_init() call that
one with a NULL argument.
I would even go a step further into the other direction. The shmem
backed GEM object is just some special handling as far as I can see.
So I would rather suggest to rename all drm_gem_* function which only
deal with the shmem backed GEM object into drm_gem_shmem_*.
Also the explanation why a different mount point helps with something
isn't very satisfying.
Regards,
Christian.
Regards,
Tvrtko
Cc: Russell King
Cc: Lucas Stach
Cc: Christian Gmeiner
Cc: Inki Dae
Cc: Seung-Woo Kim
Cc: Kyungmin Park
Cc: Krzysztof Kozlowski
Cc: Alim Akhtar
Cc: Patrik Jakobsson
Cc: Sui Jingfeng
Cc: Chun-Kuang Hu
Cc: Philipp Zabel
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: Tomi Valkeinen
Cc: Gerd Hoffmann
Cc: Sandy Huang
Cc: "Heiko Stübner"
Cc: Andy Yan
Cc: Thierry Reding
Cc: Mikko Perttunen
Cc: Jonathan Hunter
Cc: Christian König
Cc: Huang Rui
Cc: Oleksandr Andrushchenko
Cc: Karolina Stolarek
Cc: Andi Shyti
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/armada/armada_gem.c | 2 +-
drivers/gpu/drm/drm_gem.c | 12 ++--
drivers/gpu/drm/drm_gem_dma_helper.c | 2 +-
drivers/gpu/drm/drm_gem_shmem_helper.c | 2 +-
drivers/gpu/drm/drm_gem_vram_helper.c | 2 +-
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +-
drivers/gpu/drm/gma500/gem.c | 2 +-
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 +-
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
drivers/gpu/drm/msm/msm_gem.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 2 +-
drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
drivers/gpu/drm/qxl/qxl_object.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 +-
drivers/gpu/drm/tegra/gem.c | 2 +-
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 2 +-
drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
include/drm/drm_gem.h | 3 ++-
20 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c
b/drivers/gpu/drm/armada/armada_gem.c
index 26d10065d534..36a25e667341 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -226,7 +226,7 @@ static struct armada_gem_object
*armada_gem_alloc_object(struct drm_device *dev,
obj->obj.funcs = _gem_object_funcs;
- if (drm_gem_object_init(dev, >obj, size)) {
+ if (drm_gem_object_init(dev, >obj, size, NULL)) {
kfree(obj);
return NULL;
}
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 44a948b80ee1..ddd8777fcda5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -118,18 +118,26 @@ drm_gem_init(struct drm_device *dev)
* @dev: drm_device the object should be initialized for
* @obj: drm_gem_object to initialize
* @size: object size
+ * @gemfs: tmpfs mount where the GEM object will be created. If
NULL, use
+ * the usual tmpfs mountpoint (`shm_mnt`).
*
* Initialize an already allocated GEM object of the specified size
with
* shmfs backing store.
*/
int drm_gem_object_init(struct drm_device *dev,
- struct drm_gem_object *obj, size_t size)
+ struct drm_gem_object *obj, size_t size,
+ struct vfsmount *gemfs)
{
struct file *filp;
drm_gem_private_object_init(dev, obj, size);
- filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
+ if (gemfs)
+ filp = shmem_file_setup_with_mnt(gemfs, "drm mm object", size,
+ VM_NORESERVE);
+ else
+ filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
+
if (IS_ERR(filp))
return PTR_ERR(filp);
diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c
b/drivers/gpu/drm/drm_gem_dma_helper.c
index 870b90b78bc4..9ada5ac85dd6 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++
Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()
Hi Maira,
On 11/03/2024 10:05, Maíra Canal wrote:
For some applications, such as using huge pages, we might want to have a
different mountpoint, for which we pass in mount flags that better match
our usecase.
Therefore, add a new parameter to drm_gem_object_init() that allow us to
define the tmpfs mountpoint where the GEM object will be created. If
this parameter is NULL, then we fallback to shmem_file_setup().
One strategy for reducing churn, and so the number of drivers this patch
touches, could be to add a lower level drm_gem_object_init() (which
takes vfsmount, call it __drm_gem_object_init(), or
drm__gem_object_init_mnt(), and make drm_gem_object_init() call that one
with a NULL argument.
Regards,
Tvrtko
Cc: Russell King
Cc: Lucas Stach
Cc: Christian Gmeiner
Cc: Inki Dae
Cc: Seung-Woo Kim
Cc: Kyungmin Park
Cc: Krzysztof Kozlowski
Cc: Alim Akhtar
Cc: Patrik Jakobsson
Cc: Sui Jingfeng
Cc: Chun-Kuang Hu
Cc: Philipp Zabel
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: Tomi Valkeinen
Cc: Gerd Hoffmann
Cc: Sandy Huang
Cc: "Heiko Stübner"
Cc: Andy Yan
Cc: Thierry Reding
Cc: Mikko Perttunen
Cc: Jonathan Hunter
Cc: Christian König
Cc: Huang Rui
Cc: Oleksandr Andrushchenko
Cc: Karolina Stolarek
Cc: Andi Shyti
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/armada/armada_gem.c | 2 +-
drivers/gpu/drm/drm_gem.c | 12 ++--
drivers/gpu/drm/drm_gem_dma_helper.c | 2 +-
drivers/gpu/drm/drm_gem_shmem_helper.c| 2 +-
drivers/gpu/drm/drm_gem_vram_helper.c | 2 +-
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +-
drivers/gpu/drm/gma500/gem.c | 2 +-
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 +-
drivers/gpu/drm/mediatek/mtk_drm_gem.c| 2 +-
drivers/gpu/drm/msm/msm_gem.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 2 +-
drivers/gpu/drm/omapdrm/omap_gem.c| 2 +-
drivers/gpu/drm/qxl/qxl_object.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 +-
drivers/gpu/drm/tegra/gem.c | 2 +-
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 2 +-
drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
include/drm/drm_gem.h | 3 ++-
20 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c
b/drivers/gpu/drm/armada/armada_gem.c
index 26d10065d534..36a25e667341 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -226,7 +226,7 @@ static struct armada_gem_object
*armada_gem_alloc_object(struct drm_device *dev,
obj->obj.funcs = _gem_object_funcs;
- if (drm_gem_object_init(dev, >obj, size)) {
+ if (drm_gem_object_init(dev, >obj, size, NULL)) {
kfree(obj);
return NULL;
}
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 44a948b80ee1..ddd8777fcda5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -118,18 +118,26 @@ drm_gem_init(struct drm_device *dev)
* @dev: drm_device the object should be initialized for
* @obj: drm_gem_object to initialize
* @size: object size
+ * @gemfs: tmpfs mount where the GEM object will be created. If NULL, use
+ * the usual tmpfs mountpoint (`shm_mnt`).
*
* Initialize an already allocated GEM object of the specified size with
* shmfs backing store.
*/
int drm_gem_object_init(struct drm_device *dev,
- struct drm_gem_object *obj, size_t size)
+ struct drm_gem_object *obj, size_t size,
+ struct vfsmount *gemfs)
{
struct file *filp;
drm_gem_private_object_init(dev, obj, size);
- filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
+ if (gemfs)
+ filp = shmem_file_setup_with_mnt(gemfs, "drm mm object", size,
+VM_NORESERVE);
+ else
+ filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
+
if (IS_ERR(filp))
return PTR_ERR(filp);
diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c
b/drivers/gpu/drm/drm_gem_dma_helper.c
index 870b90b78bc4..9ada5ac85dd6 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -95,7 +95,7 @@ __drm_gem_dma_create(struct drm_device *drm, size_t size,
bool private)
/* Always use writecombine for dma-buf mappings */
dma_obj->map_noncoherent = false;
} else {
- ret = drm_gem_object_init(drm, gem_obj, size);
+ ret =
