On Mon, May 19, 2025 at 10:51:25AM -0700, Rob Clark wrote:
> From: Rob Clark <robdcl...@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 <d...@kernel.org>
> Signed-off-by: Rob Clark <robdcl...@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/
