Re: [PATCH v5 02/40] drm/gpuvm: Allow VAs to hold soft reference to BOs

[Danilo Krummrich <dakr@kernel.org>]

On Mon, May 19, 2025 at 10:51:25AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Eases migration for drivers where VAs don't hold hard references to
> their associated BO, avoiding reference loops.
> 
> In particular, msm uses soft references to optimistically keep around
> mappings until the BO is distroyed.  Which obviously won't work if the
> VA (the mapping) is holding a reference to the BO.
> 
> By making this a per-VM flag, we can use normal hard-references for
> mappings in a "VM_BIND" managed VM, but soft references in other cases,
> such as kernel-internal VMs (for display scanout, etc).
> 
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/drm_gpuvm.c | 37 ++++++++++++++++++++++++++++++++-----
>  include/drm/drm_gpuvm.h     | 19 +++++++++++++++++--
>  2 files changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 1e89a98caad4..892b62130ff8 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -1125,6 +1125,8 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>  	LIST_HEAD(extobjs);
>  	int ret = 0;
>  
> +	WARN_ON(gpuvm->flags & DRM_GPUVM_VA_WEAK_REF);

No, here and below, please don't scatter WARN_ON() calls in various code paths
for this hack.

This won't ever be a valid and supported mode, please keep the diff as small as
possible.

<snip>

> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 2a9629377633..652e0fb66413 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -205,10 +205,25 @@ enum drm_gpuvm_flags {
>  	 */
>  	DRM_GPUVM_RESV_PROTECTED = BIT(0),
>  
> +	/**
> +	 * @DRM_GPUVM_VA_WEAK_REF:
> +	 *
> +	 * Flag indicating that the &drm_gpuva (or more correctly, the
> +	 * &drm_gpuvm_bo) only holds a weak reference to the &drm_gem_object.
> +	 * This mode is intended to ease migration to drm_gpuvm for drivers
> +	 * where the GEM object holds a referece to the VA, rather than the
> +	 * other way around.
> +	 *
> +	 * In this mode, drm_gpuvm does not track evicted or external objects.
> +	 * It is intended for legacy mode, where the needed objects are attached
> +	 * to the command submission ioctl, therefore this tracking is unneeded.
> +	 */
> +	DRM_GPUVM_VA_WEAK_REF = BIT(1),

As mentioned in v4, I do *not* agree with making this a valid and supported
mode. If at all, it should be an exception for MSM, i.e.
DRM_GPUVM_MSM_LEGACY_QUIRK with an explicit approval from Dave / Sima [1].

It invalidates the whole design and makes a lot of functions fundamentally
invalid to call, which is well demonstrated by all the WARN_ON() calls this
patch attempts to add.

> +
>  	/**
>  	 * @DRM_GPUVM_USERBITS: user defined bits
>  	 */
> -	DRM_GPUVM_USERBITS = BIT(1),
> +	DRM_GPUVM_USERBITS = BIT(2),
>  };

[1] https://lore.kernel.org/dri-devel/aCb-72KH-NrzvGXy@pollux/