From: Boris Brezillon <boris.brezillon@collabora.com>
We are going to add flags/properties that will impact the VA merging
ability. Instead of sprinkling tests all over the place in
__drm_gpuvm_sm_map(), let's add a helper aggregating all these checks
can call it for every existing VA we walk through in the
__drm_gpuvm_sm_map() loop.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>
---
drivers/gpu/drm/drm_gpuvm.c | 46 +++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index 3c2b6102e818..4af7b71abcb4 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -2378,16 +2378,47 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
return fn->sm_step_unmap(&op, priv);
}
+static bool can_merge(struct drm_gpuvm *gpuvm, const struct drm_gpuva *va,
+ const struct drm_gpuva_op_map *new_map)
+{
+ struct drm_gpuva_op_map existing_map = {
+ .va.addr = va->va.addr,
+ .va.range = va->va.range,
+ .gem.offset = va->gem.offset,
+ .gem.obj = va->gem.obj,
+ };
+ const struct drm_gpuva_op_map *a = new_map, *b = &existing_map;
+
+ /* Only GEM-based mappings can be merged, and they must point to
+ * the same GEM object.
+ */
+ if (a->gem.obj != b->gem.obj || !a->gem.obj)
+ return false;
+
+ /* Order VAs for the rest of the checks. */
+ if (a->va.addr > b->va.addr)
+ swap(a, b);
+
+ /* We assume the caller already checked that VAs overlap or are
+ * contiguous.
+ */
+ if (drm_WARN_ON(gpuvm->drm, b->va.addr > a->va.addr + a->va.range))
+ return false;
+
+ /* We intentionally ignore u64 underflows because all we care about
+ * here is whether the VA diff matches the GEM offset diff.
+ */
+ return b->va.addr - a->va.addr == b->gem.offset - a->gem.offset;
+}
+
static int
__drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
const struct drm_gpuvm_ops *ops, void *priv,
const struct drm_gpuvm_map_req *req,
bool madvise)
{
- struct drm_gem_object *req_obj = req->map.gem.obj;
const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
struct drm_gpuva *va, *next;
- u64 req_offset = req->map.gem.offset;
u64 req_range = req->map.va.range;
u64 req_addr = req->map.va.addr;
u64 req_end = req_addr + req_range;
@@ -2402,15 +2433,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
u64 addr = va->va.addr;
u64 range = va->va.range;
u64 end = addr + range;
- bool merge = !!va->gem.obj;
+ bool merge = can_merge(gpuvm, va, &req->map);
if (madvise && obj)
continue;
if (addr == req_addr) {
- merge &= obj == req_obj &&
- offset == req_offset;
-
if (end == req_end) {
ret = op_unmap_cb(ops, priv, va, merge, madvise);
if (ret)
@@ -2455,8 +2483,6 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
};
struct drm_gpuva_op_unmap u = { .va = va };
- merge &= obj == req_obj &&
- offset + ls_range == req_offset;
u.keep = merge;
if (end == req_end) {
@@ -2506,10 +2532,6 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
break;
}
} else if (addr > req_addr) {
- merge &= obj == req_obj &&
- offset == req_offset +
- (addr - req_addr);
-
if (end == req_end) {
ret = op_unmap_cb(ops, priv, va, merge, madvise);
if (ret)
--
2.53.0On Fri, Mar 13, 2026 at 03:09:41PM +0000, Adrián Larumbe wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > > We are going to add flags/properties that will impact the VA merging > ability. Instead of sprinkling tests all over the place in > __drm_gpuvm_sm_map(), let's add a helper aggregating all these checks > can call it for every existing VA we walk through in the > __drm_gpuvm_sm_map() loop. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com> > + /* We intentionally ignore u64 underflows because all we care about > + * here is whether the VA diff matches the GEM offset diff. > + */ > + return b->va.addr - a->va.addr == b->gem.offset - a->gem.offset; Please use wrapping_sub() to make it clear to sanitizers etc that underflow is intentional in this case. Alice
On Fri Mar 13, 2026 at 4:09 PM CET, Adrián Larumbe wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
>
> We are going to add flags/properties that will impact the VA merging
> ability. Instead of sprinkling tests all over the place in
> __drm_gpuvm_sm_map(), let's add a helper aggregating all these checks
> can call it for every existing VA we walk through in the
> __drm_gpuvm_sm_map() loop.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>
This needs your Signed-off-by: as well. Does it need Caterina's Co-developed-by:
tag as well?
> ---
> drivers/gpu/drm/drm_gpuvm.c | 46 +++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 3c2b6102e818..4af7b71abcb4 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2378,16 +2378,47 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
> return fn->sm_step_unmap(&op, priv);
> }
>
> +static bool can_merge(struct drm_gpuvm *gpuvm, const struct drm_gpuva *va,
> + const struct drm_gpuva_op_map *new_map)
Not public, but please add some documentation regardless; it's not very obvious
what the function should achieve semantically from just looking at its
signature.
> +{
> + struct drm_gpuva_op_map existing_map = {
> + .va.addr = va->va.addr,
> + .va.range = va->va.range,
> + .gem.offset = va->gem.offset,
> + .gem.obj = va->gem.obj,
> + };
IIRC, previously this was a temporary struct drm_gpuva; this seems better (also
because its scope is limited to this function), but it still feels like an
abuse of this structure.
Anyways, I get that you want it for the swap() trick below, but I think it can
also easily be done without the swap() trick. What about this?
static bool can_merge(struct drm_gpuvm *gpuvm, const struct drm_gpuva *va,
const struct drm_gpuva_op_map *new_map)
{
/* Only GEM-based mappings can be merged, and they must point to
* the same GEM object.
*/
if (va->gem.obj != new_map->gem.obj || !new_map->gem.obj)
return false;
/* We assume the caller already checked that VAs overlap or are
* contiguous.
*/
if (drm_WARN_ON(gpuvm->drm,
new_map->va.addr > va->va.addr + va->va.range ||
va->va.addr > new_map->va.addr + new_map->va.range))
return false;
/* u64 underflow is fine: both sides negate equally, preserving
* the equality.
*/
return va->va.addr - new_map->va.addr ==
va->gem.offset - new_map->gem.offset;
}
> + const struct drm_gpuva_op_map *a = new_map, *b = &existing_map;
> +
> + /* Only GEM-based mappings can be merged, and they must point to
> + * the same GEM object.
> + */
> + if (a->gem.obj != b->gem.obj || !a->gem.obj)
> + return false;
> +
> + /* Order VAs for the rest of the checks. */
> + if (a->va.addr > b->va.addr)
> + swap(a, b);
> +
> + /* We assume the caller already checked that VAs overlap or are
> + * contiguous.
> + */
> + if (drm_WARN_ON(gpuvm->drm, b->va.addr > a->va.addr + a->va.range))
> + return false;
> +
> + /* We intentionally ignore u64 underflows because all we care about
> + * here is whether the VA diff matches the GEM offset diff.
> + */
> + return b->va.addr - a->va.addr == b->gem.offset - a->gem.offset;
> +}
© 2016 - 2026 Red Hat, Inc.