[PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()

[Daniel Vetter]

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 at 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch