From: Mary Guillemard <mary@mary.zone>
Now that everything in UVMM knows about the variable page shift, we can
select larger values.
The proposed approach rely on nouveau_bo::page unless it would cause
alignment issues (in which case we fall back to searching an appropriate
shift)
Co-developed-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
Signed-off-by: Mary Guillemard <mary@mary.zone>
---
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 55 +++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index a92c729600d6..c336a121e320 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -454,6 +454,56 @@ op_unmap_prepare_unwind(struct drm_gpuva *va)
drm_gpuva_insert(va->vm, va);
}
+static bool
+op_map_aligned_to_page_shift(const struct drm_gpuva_op_map *op, u8 page_shift)
+{
+ u64 page_size = 1ULL << page_shift;
+
+ return op->va.addr % page_size == 0 && op->va.range % page_size == 0 &&
+ op->gem.offset % page_size == 0;
+}
+
+static u8
+select_page_shift(struct nouveau_uvmm *uvmm, struct drm_gpuva_op_map *op)
+{
+ struct nouveau_bo *nvbo = nouveau_gem_object(op->gem.obj);
+
+ if (nvbo) {
+ /* If the BO preferred page shift already fits, use it. */
+ if (op_map_aligned_to_page_shift(op, nvbo->page))
+ return nvbo->page;
+
+ struct nouveau_mem *mem = nouveau_mem(nvbo->bo.resource);
+ struct nvif_vmm *vmm = &uvmm->vmm.vmm;
+ int i;
+
+ /* Otherwise let's find a granuality that will fit. */
+ for (i = 0; i < vmm->page_nr; i++) {
+ /* Ignore anything that is bigger or identical to the BO preference. */
+ if (vmm->page[i].shift >= nvbo->page)
+ continue;
+
+ /* Skip incompatible domains. */
+ if ((mem->mem.type & NVIF_MEM_VRAM) && !vmm->page[i].vram)
+ continue;
+ if ((mem->mem.type & NVIF_MEM_HOST) &&
+ (!vmm->page[i].host || vmm->page[i].shift > PAGE_SHIFT))
+ continue;
+
+ /* If it fits, return the proposed shift. */
+ if (op_map_aligned_to_page_shift(op, vmm->page[i].shift))
+ return vmm->page[i].shift;
+ }
+
+ /* If we get here then nothing can reconcile the requirements. This should never
+ * happen.
+ */
+ WARN_ON(1);
+ }
+
+ return PAGE_SHIFT;
+}
+
static void
nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
struct nouveau_uvma_prealloc *new,
@@ -506,7 +556,7 @@ nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
if (vmm_get_range)
nouveau_uvmm_vmm_put(uvmm, vmm_get_start,
vmm_get_range,
- PAGE_SHIFT);
+ select_page_shift(uvmm, &op->map));
break;
}
case DRM_GPUVA_OP_REMAP: {
@@ -636,7 +686,8 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
case DRM_GPUVA_OP_MAP: {
u64 vmm_get_range = vmm_get_end - vmm_get_start;
- ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT);
+ ret = op_map_prepare(uvmm, &new->map, &op->map, args,
+ select_page_shift(uvmm, &op->map));
if (ret)
goto unwind;
--
2.51.0On Mon Oct 6, 2025 at 9:13 PM CEST, Mohamed Ahmed wrote:
> From: Mary Guillemard <mary@mary.zone>
>
> Now that everything in UVMM knows about the variable page shift, we can
> select larger values.
>
> The proposed approach rely on nouveau_bo::page unless it would cause
> alignment issues (in which case we fall back to searching an appropriate
> shift)
>
> Co-developed-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
> Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
> Signed-off-by: Mary Guillemard <mary@mary.zone>
NIT: Both of your tags should come after Mary's tag. The same applied to some of
the other patches.
> ---
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 55 +++++++++++++++++++++++++-
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index a92c729600d6..c336a121e320 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -454,6 +454,56 @@ op_unmap_prepare_unwind(struct drm_gpuva *va)
> drm_gpuva_insert(va->vm, va);
> }
>
> +static bool
> +op_map_aligned_to_page_shift(const struct drm_gpuva_op_map *op, u8 page_shift)
> +{
> + u64 page_size = 1ULL << page_shift;
> +
> + return op->va.addr % page_size == 0 && op->va.range % page_size == 0 &&
> + op->gem.offset % page_size == 0;
> +}
> +
> +static u8
> +select_page_shift(struct nouveau_uvmm *uvmm, struct drm_gpuva_op_map *op)
> +{
> + struct nouveau_bo *nvbo = nouveau_gem_object(op->gem.obj);
> +
> + if (nvbo) {
In nouveau a struct drm_gpuva_op_map always has a valid GEM object set; we bail
out if userspace gives us an invalid GEM handle.
> + /* If the BO preferred page shift already fits, use it. */
> + if (op_map_aligned_to_page_shift(op, nvbo->page))
> + return nvbo->page;
> +
> + struct nouveau_mem *mem = nouveau_mem(nvbo->bo.resource);
> + struct nvif_vmm *vmm = &uvmm->vmm.vmm;
> + int i;
> +
> + /* Otherwise let's find a granuality that will fit. */
Do we ever run into the "otherwise" case? nouveau_bo_fixup_align() seems to
already ensure that your previous call will never fail?
> + for (i = 0; i < vmm->page_nr; i++) {
> + /* Ignore anything that is bigger or identical to the BO preference. */
> + if (vmm->page[i].shift >= nvbo->page)
> + continue;
> +
> + /* Skip incompatible domains. */
> + if ((mem->mem.type & NVIF_MEM_VRAM) && !vmm->page[i].vram)
> + continue;
> + if ((mem->mem.type & NVIF_MEM_HOST) &&
> + (!vmm->page[i].host || vmm->page[i].shift > PAGE_SHIFT))
> + continue;
> +
> + /* If it fits, return the proposed shift. */
> + if (op_map_aligned_to_page_shift(op, vmm->page[i].shift))
> + return vmm->page[i].shift;
> + }
> +
> + /* If we get here then nothing can reconcile the requirements. This should never
> + * happen.
> + */
> + WARN_ON(1);
> + }
> +
> + return PAGE_SHIFT;
> +}
> +
> static void
> nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> struct nouveau_uvma_prealloc *new,
> @@ -506,7 +556,7 @@ nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> if (vmm_get_range)
> nouveau_uvmm_vmm_put(uvmm, vmm_get_start,
> vmm_get_range,
> - PAGE_SHIFT);
> + select_page_shift(uvmm, &op->map));
> break;
> }
> case DRM_GPUVA_OP_REMAP: {
> @@ -636,7 +686,8 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
> case DRM_GPUVA_OP_MAP: {
> u64 vmm_get_range = vmm_get_end - vmm_get_start;
>
> - ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT);
> + ret = op_map_prepare(uvmm, &new->map, &op->map, args,
> + select_page_shift(uvmm, &op->map));
Let's move the call to select_page_shift() into op_map_prepare().
> if (ret)
> goto unwind;
>
> --
> 2.51.0
Hey,
Thank you so much for the review! I applied everything for v2, except this one:
> Let's move the call to select_page_shift() into op_map_prepare().
How would this work? Originally when we were working on this, we did
place it in op_map_prepare() but we ran into the issue where
nouveau_uvmm_vmm_put() needed the page_shift retrieved (see
nouveau_uvmm_sm_prepare_unwind()).
Thanks.
On Mon, Oct 6, 2025 at 11:26 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Oct 6, 2025 at 9:13 PM CEST, Mohamed Ahmed wrote:
> > From: Mary Guillemard <mary@mary.zone>
> >
> > Now that everything in UVMM knows about the variable page shift, we can
> > select larger values.
> >
> > The proposed approach rely on nouveau_bo::page unless it would cause
> > alignment issues (in which case we fall back to searching an appropriate
> > shift)
> >
> > Co-developed-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
> > Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>
> > Signed-off-by: Mary Guillemard <mary@mary.zone>
>
> NIT: Both of your tags should come after Mary's tag. The same applied to some of
> the other patches.
>
> > ---
> > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 55 +++++++++++++++++++++++++-
> > 1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > index a92c729600d6..c336a121e320 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > @@ -454,6 +454,56 @@ op_unmap_prepare_unwind(struct drm_gpuva *va)
> > drm_gpuva_insert(va->vm, va);
> > }
> >
> > +static bool
> > +op_map_aligned_to_page_shift(const struct drm_gpuva_op_map *op, u8 page_shift)
> > +{
> > + u64 page_size = 1ULL << page_shift;
> > +
> > + return op->va.addr % page_size == 0 && op->va.range % page_size == 0 &&
> > + op->gem.offset % page_size == 0;
> > +}
> > +
> > +static u8
> > +select_page_shift(struct nouveau_uvmm *uvmm, struct drm_gpuva_op_map *op)
> > +{
> > + struct nouveau_bo *nvbo = nouveau_gem_object(op->gem.obj);
> > +
> > + if (nvbo) {
>
> In nouveau a struct drm_gpuva_op_map always has a valid GEM object set; we bail
> out if userspace gives us an invalid GEM handle.
>
> > + /* If the BO preferred page shift already fits, use it. */
> > + if (op_map_aligned_to_page_shift(op, nvbo->page))
> > + return nvbo->page;
> > +
> > + struct nouveau_mem *mem = nouveau_mem(nvbo->bo.resource);
> > + struct nvif_vmm *vmm = &uvmm->vmm.vmm;
> > + int i;
> > +
> > + /* Otherwise let's find a granuality that will fit. */
>
> Do we ever run into the "otherwise" case? nouveau_bo_fixup_align() seems to
> already ensure that your previous call will never fail?
>
> > + for (i = 0; i < vmm->page_nr; i++) {
> > + /* Ignore anything that is bigger or identical to the BO preference. */
> > + if (vmm->page[i].shift >= nvbo->page)
> > + continue;
> > +
> > + /* Skip incompatible domains. */
> > + if ((mem->mem.type & NVIF_MEM_VRAM) && !vmm->page[i].vram)
> > + continue;
> > + if ((mem->mem.type & NVIF_MEM_HOST) &&
> > + (!vmm->page[i].host || vmm->page[i].shift > PAGE_SHIFT))
> > + continue;
> > +
> > + /* If it fits, return the proposed shift. */
> > + if (op_map_aligned_to_page_shift(op, vmm->page[i].shift))
> > + return vmm->page[i].shift;
> > + }
> > +
> > + /* If we get here then nothing can reconcile the requirements. This should never
> > + * happen.
> > + */
> > + WARN_ON(1);
> > + }
> > +
> > + return PAGE_SHIFT;
> > +}
> > +
> > static void
> > nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> > struct nouveau_uvma_prealloc *new,
> > @@ -506,7 +556,7 @@ nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> > if (vmm_get_range)
> > nouveau_uvmm_vmm_put(uvmm, vmm_get_start,
> > vmm_get_range,
> > - PAGE_SHIFT);
> > + select_page_shift(uvmm, &op->map));
> > break;
> > }
> > case DRM_GPUVA_OP_REMAP: {
> > @@ -636,7 +686,8 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
> > case DRM_GPUVA_OP_MAP: {
> > u64 vmm_get_range = vmm_get_end - vmm_get_start;
> >
> > - ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT);
> > + ret = op_map_prepare(uvmm, &new->map, &op->map, args,
> > + select_page_shift(uvmm, &op->map));
>
> Let's move the call to select_page_shift() into op_map_prepare().
>
> > if (ret)
> > goto unwind;
> >
> > --
> > 2.51.0
>
On Thu Oct 9, 2025 at 6:51 PM CEST, Mohamed Ahmed wrote: >> Let's move the call to select_page_shift() into op_map_prepare(). > > How would this work? Originally when we were working on this, we did > place it in op_map_prepare() but we ran into the issue where > nouveau_uvmm_vmm_put() needed the page_shift retrieved (see > nouveau_uvmm_sm_prepare_unwind()). - ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT); + ret = op_map_prepare(uvmm, &new->map, &op->map, args, + select_page_shift(uvmm, &op->map)); You can move this call to select_page_shift() into op_map_prepare(), that's not related to nouveau_uvmm_sm_prepare_unwind(), right?
Sorry about that, I misunderstood what you meant and thought you meant to remove select_page_shift() entirely and move the shift selection logic into op_map_prepare(). Done and sent v2. On Thu, Oct 9, 2025 at 11:09 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Thu Oct 9, 2025 at 6:51 PM CEST, Mohamed Ahmed wrote: > >> Let's move the call to select_page_shift() into op_map_prepare(). > > > > How would this work? Originally when we were working on this, we did > > place it in op_map_prepare() but we ran into the issue where > > nouveau_uvmm_vmm_put() needed the page_shift retrieved (see > > nouveau_uvmm_sm_prepare_unwind()). > > - ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT); > + ret = op_map_prepare(uvmm, &new->map, &op->map, args, > + select_page_shift(uvmm, &op->map)); > > You can move this call to select_page_shift() into op_map_prepare(), that's not > related to nouveau_uvmm_sm_prepare_unwind(), right?
© 2016 - 2025 Red Hat, Inc.