Alexandre Courbot
2025-Nov-26 13:22 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
On Wed Nov 26, 2025 at 6:54 PM JST, Alice Ryhl wrote:> On Wed, Nov 26, 2025 at 09:31:46AM +0900, Alexandre Courbot wrote: >> On Tue Nov 25, 2025 at 11:59 PM JST, Alice Ryhl wrote: >> > 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. >> >> Isn't PAGE_SIZE always a power of two though? > > Yes it is. Maybe you can elaborate on how you wanted to use Alignment? > It sounds like you have something different in mind than what I thought.I thought we could just do something like this: use kernel::ptr::{Alignable, Alignment}; let aligned_size = size .align_up(Alignment::new::<PAGE_SIZE>()) .ok_or(EOVERFLOW)?; (maybe we could also have that `Alignment<PAGE_SIZE>` stored as a const in `page.rs` for convenience, as it might be used often)
Alice Ryhl
2025-Nov-26 13:36 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
On Wed, Nov 26, 2025 at 10:22:14PM +0900, Alexandre Courbot wrote:> On Wed Nov 26, 2025 at 6:54 PM JST, Alice Ryhl wrote: > > On Wed, Nov 26, 2025 at 09:31:46AM +0900, Alexandre Courbot wrote: > >> On Tue Nov 25, 2025 at 11:59 PM JST, Alice Ryhl wrote: > >> > 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: > >> >> >> > 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. > >> > >> Isn't PAGE_SIZE always a power of two though? > > > > Yes it is. Maybe you can elaborate on how you wanted to use Alignment? > > It sounds like you have something different in mind than what I thought. > > I thought we could just do something like this: > > use kernel::ptr::{Alignable, Alignment}; > > let aligned_size = size > .align_up(Alignment::new::<PAGE_SIZE>()) > .ok_or(EOVERFLOW)?; > > (maybe we could also have that `Alignment<PAGE_SIZE>` stored as a const > in `page.rs` for convenience, as it might be used often)If you're trying to round up a number to the next multiple of PAGE_SIZE, then you should use page_align() because that's exactly what the function does. If you think there is something wrong with the design of page_align(), change it instead of reimplemtning it. Alice