This patchset fixes two problems in the common GEM code. First fixed problem
is the bogus lockdep splat that complicates debugging of DRM drivers. Second
problem is the inconsistency in behaviour and improper handling of mapping
the imported GEMs by some drivers, to fix it we will prohibit to map the
imported GEMs like majority of drivers already do.
Changelog:
v7: - Factored out GEM patches from [1] since I'll be working on the
dma-buf locking in a separate patchsets now.
[1] https://lore.kernel.org/all/[email protected]/
- Improved commit message and added fixes tag to the "Properly annotate
WW context" patch.
- Replaced "Move mapping of imported dma-bufs to drm_gem_mmap_obj()"
patch with "Don't map imported GEMs", like was suggested by
Thomas Hellström.
- Added r-b and suggested-by from Thomas Hellström.
Dmitry Osipenko (2):
drm/gem: Properly annotate WW context on drm_gem_lock_reservations()
error
drm/gem: Don't map imported GEMs
drivers/gpu/drm/drm_gem.c | 8 ++++++--
drivers/gpu/drm/drm_gem_shmem_helper.c | 9 ---------
2 files changed, 6 insertions(+), 11 deletions(-)
--
2.36.1
Use ww_acquire_fini() in the error code paths. Otherwise lockdep
thinks that lock is held when lock's memory is freed after the
drm_gem_lock_reservations() error. The ww_acquire_context needs to be
annotated as "released", which fixes the noisy "WARNING: held lock freed!"
splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
Cc: [email protected]
Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
Reviewed-by: Thomas Hellström <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/drm_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index eb0c2d041f13..86d670c71286 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
ret = dma_resv_lock_slow_interruptible(obj->resv,
acquire_ctx);
if (ret) {
- ww_acquire_done(acquire_ctx);
+ ww_acquire_fini(acquire_ctx);
return ret;
}
}
@@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
goto retry;
}
- ww_acquire_done(acquire_ctx);
+ ww_acquire_fini(acquire_ctx);
return ret;
}
}
--
2.36.1
On Thu, Jun 30, 2022 at 11:04:04PM +0300, Dmitry Osipenko wrote:
> Use ww_acquire_fini() in the error code paths. Otherwise lockdep
> thinks that lock is held when lock's memory is freed after the
> drm_gem_lock_reservations() error. The ww_acquire_context needs to be
> annotated as "released", which fixes the noisy "WARNING: held lock freed!"
> splat of VirtIO-GPU driver with CONFIG_DEBUG_MUTEXES=y and enabled lockdep.
>
> Cc: [email protected]
> Fixes: 7edc3e3b975b5 ("drm: Add helpers for locking an array of BO reservations.")
> Reviewed-by: Thomas Hellstr?m <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
I merged this one to drm-misc-next-fixes. The other one looks like there's
still opens pending, pls resubmit appropriately (and maybe with some
analysis in the commit message of how exactly this impacts other drivers).
-Daniel
> ---
> drivers/gpu/drm/drm_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..86d670c71286 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1226,7 +1226,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> ret = dma_resv_lock_slow_interruptible(obj->resv,
> acquire_ctx);
> if (ret) {
> - ww_acquire_done(acquire_ctx);
> + ww_acquire_fini(acquire_ctx);
> return ret;
> }
> }
> @@ -1251,7 +1251,7 @@ drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> goto retry;
> }
>
> - ww_acquire_done(acquire_ctx);
> + ww_acquire_fini(acquire_ctx);
> return ret;
> }
> }
> --
> 2.36.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch