Alexandre Courbot
2025-Nov-25 14:55 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
On Tue Nov 25, 2025 at 11:41 PM JST, Alice Ryhl wrote:> On Tue, Nov 25, 2025 at 3:29?PM Alexandre Courbot <acourbot at nvidia.com> wrote: >> >> On Fri Nov 21, 2025 at 1:04 PM JST, bshephar wrote: >> > Use page::page_align for GEM object memory allocation to ensure the >> > allocation is page aligned. This ensures that the allocation is page >> > aligned with the system in cases where 4096 is not the default. >> > For example on 16k or 64k aarch64 systems this allocation should be >> > aligned accordingly. >> > >> > Signed-off-by: Brendan Shephard <bshephar at bne-home.net> >> > --- >> > drivers/gpu/drm/nova/gem.rs | 11 ++++++++--- >> > 1 file changed, 8 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs >> > index 2760ba4f3450..a07e922e25ef 100644 >> > --- a/drivers/gpu/drm/nova/gem.rs >> > +++ b/drivers/gpu/drm/nova/gem.rs >> > @@ -3,6 +3,10 @@ >> > use kernel::{ >> > drm, >> > drm::{gem, gem::BaseObject}, >> > + page::{ >> > + page_align, >> > + PAGE_SIZE, // >> > + }, >> > prelude::*, >> > sync::aref::ARef, >> > }; >> > @@ -27,12 +31,13 @@ fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> { >> > impl NovaObject { >> > /// Create a new DRM GEM object. >> > pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self>>> { >> > - let aligned_size = size.next_multiple_of(1 << 12); >> > - >> > - if size == 0 || size > aligned_size { >> > + // Check for 0 size or potential usize overflow before calling page_align >> > + if size == 0 || size > usize::MAX - PAGE_SIZE + 1 { >> >> `PAGE_SIZE` here is no more correct than the hardcoded `1 << 12` - well, >> I'll admit it looks better as a placeholder. :) But the actual alignment >> will eventually be provided elsewhere. > > What about kernels with 16k pages?The actual alignment should IIUC be a mix of the GPU and kernel's requirements (GPU can also use a different page size). So no matter what we pick right now, it won't be great but you are right that PAGE_SIZE will at least accomodate the kernel.> >> > return Err(EINVAL); >> > } >> > >> > + let aligned_size = page_align(size); >> >> `page_align` won't panic on overflow, but it will still return an >> invalid size. This is a job for `kernel::ptr::Alignment`, which let's >> you return an error when an overflow occurs. > > The Rust implementation of page_align() is implemented as (addr + > (PAGE_SIZE - 1)) & PAGE_MASK, which definitely will panic on overflow > if the appropriate config options are enabled.That's right, I skimmed its code too fast. ^_^; All the more reason to use `Alignment`.
Alice Ryhl
2025-Nov-25 14:59 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
On Tue, Nov 25, 2025 at 3:55?PM Alexandre Courbot <acourbot at nvidia.com> wrote:> > On Tue Nov 25, 2025 at 11:41 PM JST, Alice Ryhl wrote: > > On Tue, Nov 25, 2025 at 3:29?PM Alexandre Courbot <acourbot at nvidia.com> wrote: > >> > >> On Fri Nov 21, 2025 at 1:04 PM JST, bshephar wrote: > >> > Use page::page_align for GEM object memory allocation to ensure the > >> > allocation is page aligned. This ensures that the allocation is page > >> > aligned with the system in cases where 4096 is not the default. > >> > For example on 16k or 64k aarch64 systems this allocation should be > >> > aligned accordingly. > >> > > >> > Signed-off-by: Brendan Shephard <bshephar at bne-home.net> > >> > --- > >> > drivers/gpu/drm/nova/gem.rs | 11 ++++++++--- > >> > 1 file changed, 8 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs > >> > index 2760ba4f3450..a07e922e25ef 100644 > >> > --- a/drivers/gpu/drm/nova/gem.rs > >> > +++ b/drivers/gpu/drm/nova/gem.rs > >> > @@ -3,6 +3,10 @@ > >> > use kernel::{ > >> > drm, > >> > drm::{gem, gem::BaseObject}, > >> > + page::{ > >> > + page_align, > >> > + PAGE_SIZE, // > >> > + }, > >> > prelude::*, > >> > sync::aref::ARef, > >> > }; > >> > @@ -27,12 +31,13 @@ fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> { > >> > impl NovaObject { > >> > /// Create a new DRM GEM object. > >> > pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self>>> { > >> > - let aligned_size = size.next_multiple_of(1 << 12); > >> > - > >> > - if size == 0 || size > aligned_size { > >> > + // Check for 0 size or potential usize overflow before calling page_align > >> > + if size == 0 || size > usize::MAX - PAGE_SIZE + 1 { > >> > >> `PAGE_SIZE` here is no more correct than the hardcoded `1 << 12` - well, > >> I'll admit it looks better as a placeholder. :) But the actual alignment > >> will eventually be provided elsewhere. > > > > What about kernels with 16k pages? > > The actual alignment should IIUC be a mix of the GPU and kernel's > requirements (GPU can also use a different page size). So no matter what > we pick right now, it won't be great but you are right that PAGE_SIZE > will at least accomodate the kernel.In that case, is PAGE_SIZE not the wrong constant? What's the actually correct constant here?> >> > return Err(EINVAL); > >> > } > >> > > >> > + let aligned_size = page_align(size); > >> > >> `page_align` won't panic on overflow, but it will still return an > >> invalid size. This is a job for `kernel::ptr::Alignment`, which let's > >> you return an error when an overflow occurs. > > > > The Rust implementation of page_align() is implemented as (addr + > > (PAGE_SIZE - 1)) & PAGE_MASK, which definitely will panic on overflow > > if the appropriate config options are enabled. > > That's right, I skimmed its code too fast. ^_^; All the more reason to > use `Alignment`.Alignment stores values that are powers of two, not multiples of PAGE_SIZE. Alice