Re: [PATCH v4 4/7] drm/gpuvm: Add a helper to check if two VA can be merged

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

On Mon Jul 7, 2025 at 9:00 PM CEST, Danilo Krummrich wrote:
> On Mon Jul 7, 2025 at 7:04 PM CEST, Caterina Shablia wrote:
>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> index 05978c5c38b1..dc3c2f906400 100644
>> --- a/drivers/gpu/drm/drm_gpuvm.c
>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>> @@ -2098,12 +2098,48 @@ 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 *a,
>> +		      const struct drm_gpuva *b)
>> +{
>> +	/* 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;
>> +
>> +	/* Let's keep things simple for now and force all flags to match. */
>> +	if (a->flags != b->flags)
>> +		return false;

Forgot to mention, this can include driver specific flags. How do we know from
the generic code whether this condition makes sense? *At least* it would need to
be documented.

However, I think it would be better to provide an optional callback for drivers
to check whether merge makes sense or not. This doesn't mean we need drivers to
do those common checks, this can remain here in the common code.

>> +
>> +	/* 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)
>>  {
>>  	struct drm_gpuva *va, *next;
>> +	struct drm_gpuva reqva = {
>> +		.va.addr = req->va.addr,
>> +		.va.range = req->va.range,
>> +		.gem.offset = req->gem.offset,
>> +		.gem.obj = req->gem.obj,
>> +		.flags = req->flags,
>
> Huh? Where does req->flags come from? I don't remember that this flag exists in
> struct drm_gpuvm_map_req in the preceding patch?
>
>> +	};
>>  	u64 req_end = req->va.addr + req->va.range;
>>  	int ret;
>>  
>> @@ -2116,12 +2152,9 @@ __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, &reqva);
>
> I know you want to do the swap() trick above, but I don't like creating a
> temporary struct drm_gpuva with all the other uninitialized fields.
>
> If you really want this, can we please limit the scope? Maybe the following
> helper:
>
> 	static bool can_merge(struct drm_gpuvm *gpuvm,
> 			      const struct drm_gpuva *va,
> 			      struct drm_gpuvm_map_req *req)
> 	{
> 		struct drm_gpuva reqva = { ... };
> 		return __can_merge(gpuvm, va, reqva);
> 	}