Re: [PATCH 2/3] drm/xe: share bo dma-resv with backup object

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Mon, 2025-04-14 at 11:32 +0100, Matthew Auld wrote:
> Hi,
> 
> On 11/04/2025 16:12, Thomas Hellström wrote:
> > On Thu, 2025-04-10 at 17:20 +0100, Matthew Auld wrote:
> > > We end up needing to grab both locks together anyway and keep
> > > them
> > > held
> > > until we complete the copy or add the fence. Plus the backup_obj
> > > is
> > > short lived and tied to the parent object, so seems reasonable to
> > > share
> > > the same dma-resv. This will simplify the locking here, and in
> > > follow
> > > up patches.
> > > 
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > 
> > Is there any chance that the bo dma-resv is freed before the backup
> > object's resv is individualized?
> > 
> > If not, perhaps a short description why that can never happen?
> 
> Thanks for reviewing. My thinking was that there should be one
> reference 
> on the backup bo, which is either dropped by the parent bo when
> calling 
> the unpin or the unprepare step, whoever gets there first. In both
> cases 
> there will still be a ref to the parent when we drop the backup ref.
> The 
> individualize step looks to be synchronous in ttm so it should happen
> within the scope of holding the parent lock so parent bo can't
> disappear.
> 
> But as you say maybe this is inviting trouble later, if there is some
> hypothetical way for something to grab an extra ref on the backup bo.
> What about if in addition we also hold a ref to the parent bo, which
> is 
> then dropped after the individualize step:
> 
> https://gitlab.freedesktop.org/mwa/kernel/-/commit/3b72a079a1cd9da9590da82912798785ede8c97f

Yes, that should work, I think. One could also try to drop the
reference in xe_tt_bo_release_notify() but that will not always work if
the individualization fails.

Thanks,
Thomas



> 
> > 
> > /Thomas
> > 
> > 
> > > ---
> > >   drivers/gpu/drm/xe/xe_bo.c | 24 +++++++++---------------
> > >   1 file changed, 9 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > > b/drivers/gpu/drm/xe/xe_bo.c
> > > index c337790c81ae..3eab6352d9dc 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -1120,9 +1120,10 @@ int xe_bo_evict_pinned(struct xe_bo *bo)
> > >   	if (bo->flags & XE_BO_FLAG_PINNED_NORESTORE)
> > >   		goto out_unlock_bo;
> > >   
> > > -	backup = xe_bo_create_locked(xe, NULL, NULL, bo->size,
> > > ttm_bo_type_kernel,
> > > -				     XE_BO_FLAG_SYSTEM |
> > > XE_BO_FLAG_NEEDS_CPU_ACCESS |
> > > -				     XE_BO_FLAG_PINNED);
> > > +	backup = ___xe_bo_create_locked(xe, NULL, NULL, bo-
> > > > ttm.base.resv, NULL, bo->size,
> > > +					DRM_XE_GEM_CPU_CACHING_W
> > > B,
> > > ttm_bo_type_kernel,
> > > +					XE_BO_FLAG_SYSTEM |
> > > XE_BO_FLAG_NEEDS_CPU_ACCESS |
> > > +					XE_BO_FLAG_PINNED);
> > >   	if (IS_ERR(backup)) {
> > >   		ret = PTR_ERR(backup);
> > >   		goto out_unlock_bo;
> > > @@ -1177,7 +1178,6 @@ int xe_bo_evict_pinned(struct xe_bo *bo)
> > >   
> > >   out_backup:
> > >   	xe_bo_vunmap(backup);
> > > -	xe_bo_unlock(backup);
> > >   	if (ret)
> > >   		xe_bo_put(backup);
> > >   out_unlock_bo:
> > > @@ -1212,17 +1212,12 @@ int xe_bo_restore_pinned(struct xe_bo
> > > *bo)
> > >   	if (!backup)
> > >   		return 0;
> > >   
> > > -	xe_bo_lock(backup, false);
> > > +	xe_bo_lock(bo, false);
> > >   
> > >   	ret = ttm_bo_validate(&backup->ttm, &backup->placement,
> > > &ctx);
> > >   	if (ret)
> > >   		goto out_backup;
> > >   
> > > -	if (WARN_ON(!dma_resv_trylock(bo->ttm.base.resv))) {
> > > -		ret = -EBUSY;
> > > -		goto out_backup;
> > > -	}
> > > -
> > >   	if (xe_bo_is_user(bo) || (bo->flags &
> > > XE_BO_FLAG_PINNED_LATE_RESTORE)) {
> > >   		struct xe_migrate *migrate;
> > >   		struct dma_fence *fence;
> > > @@ -1271,15 +1266,14 @@ int xe_bo_restore_pinned(struct xe_bo
> > > *bo)
> > >   
> > >   	bo->backup_obj = NULL;
> > >   
> > > +out_backup:
> > > +	xe_bo_vunmap(backup);
> > > +	if (!bo->backup_obj)
> > > +		xe_bo_put(backup);
> > >   out_unlock_bo:
> > >   	if (unmap)
> > >   		xe_bo_vunmap(bo);
> > >   	xe_bo_unlock(bo);
> > > -out_backup:
> > > -	xe_bo_vunmap(backup);
> > > -	xe_bo_unlock(backup);
> > > -	if (!bo->backup_obj)
> > > -		xe_bo_put(backup);
> > >   	return ret;
> > >   }
> > >   
> > 
>