[PATCH v3 0/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*()
Replace the deprecated API with new helpers, according to the TODO list of the DRM subsystem. The new API has been introduced with commit b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset locks using a local context. This has the advantage of reducing boilerplate.
The work is carried out in two consecutive logical steps: the first patch converts the code to use drm_modeset_lock_all(), then the second does the final replace with DRM_MODESET_LOCK_ALL_*().
Changes from v2: The work is split in two consecutive logical steps. Changes from v1: Added further information to the commit message.
Fabio M. De Francesco (2): drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx() drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*()
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Replace the deprecated API with new helpers, according to the TODO list of the DRM subsystem.
In this first patch drm_modeset_lock_all() is replaced with drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(), the latter doesn’t take the global drm_mode_config.mutex since that lock isn’t required for modeset state changes.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com ---
Changes from v2: The work is split in two consecutive logical steps. Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 671ec1002230..856903db34c5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) { struct drm_crtc *crtc; - - drm_modeset_lock_all(drm_dev); + struct drm_modeset_acquire_ctx ctx; + int ret_lock = 0; + +retry: + drm_modeset_lock_all_ctx(drm_dev, &ctx); + if(ret_lock == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + }
drm_for_each_crtc(crtc, drm_dev) { if (crtc->state->active) { @@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev) } }
- drm_modeset_unlock_all(drm_dev); + drm_modeset_drop_locks(&ctx);
} else { struct drm_connector *list_connector;
On Wed, Apr 21, 2021 at 01:37:15PM +0200, Fabio M. De Francesco wrote:
Replace the deprecated API with new helpers, according to the TODO list of the DRM subsystem.
In this first patch drm_modeset_lock_all() is replaced with drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(), the latter doesn’t take the global drm_mode_config.mutex since that lock isn’t required for modeset state changes.
So this is only true for core modeset code, not necessarily for drivers. So needs to be audited in each case.
Now loocking at the precise code you're touching here the situation is a lot more specific: - We are looping over all the crtc. This list never changes, so no locks needed. - Then we look at crtc->state, which is proteced by crtc->mutex. So that's the only lock we need, and we only need to hold a single one (so no EDEADLCK retry loop needed due to multiple lock acquisition).
So I think the right fix here is to just grab the crtc->mutex lock right around the access to crtc->state with drm_modeset_lock(). And ditch the lock_all and all its complexity completely.
tldr; locking is complicated :-)
Can you pls respin?
Thanks, Daniel
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
Changes from v2: The work is split in two consecutive logical steps. Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 671ec1002230..856903db34c5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) { struct drm_crtc *crtc;
drm_modeset_lock_all(drm_dev);
struct drm_modeset_acquire_ctx ctx;int ret_lock = 0;+retry:
drm_modeset_lock_all_ctx(drm_dev, &ctx);if(ret_lock == -EDEADLK) {drm_modeset_backoff(&ctx);goto retry;}drm_for_each_crtc(crtc, drm_dev) { if (crtc->state->active) {
@@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev) } }
drm_modeset_unlock_all(drm_dev);
drm_modeset_drop_locks(&ctx);} else { struct drm_connector *list_connector;
-- 2.31.1
This second patch makes use of the API that has been introduced with commit b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com ---
Changes from v2: The work is split in two consecutive logical steps. Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 856903db34c5..43dd77c227ed 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1441,12 +1441,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev) struct drm_modeset_acquire_ctx ctx; int ret_lock = 0;
-retry: - drm_modeset_lock_all_ctx(drm_dev, &ctx); - if(ret_lock == -EDEADLK) { - drm_modeset_backoff(&ctx); - goto retry; - } + DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) { if (crtc->state->active) { @@ -1455,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev) } }
- drm_modeset_drop_locks(&ctx); + DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else { struct drm_connector *list_connector;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Fabio M. De Francesco