On Fri Mar 13, 2026 at 4:09 PM CET, Adrián Larumbe wrote:
> From: Boris Brezillon <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Caterina Shablia <[email protected]>
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;
> +}
