Re: [PATCH v5 04/11] drm/gpuvm: Add a helper to check if two VA can be merged

["Danilo Krummrich" <dakr@kernel.org>]

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;
> +}