[PATCH v2 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*

[Daniel Vetter]

On Mon, Apr 19, 2021 at 05:03:40PM +0200, Fabio M. De Francesco wrote:
> 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 and has the advantage of reducing boilerplate.

So this is only half of the story, because the boilerplate only exists
when you're using drm_modeset_lock_all_ctx. Which should be used for
modern atomic display drivers everywhere.

But drm_modeset_lock_all_ctx isn't exactly the same as
drm_modeset_lock_all, so this needs to be explained.

Now the problem with drm_modeset_lock_all is that it hides a memory
allocation, and if that allocation fails then we just die. Which isn't
great really, but in practice the kernel tries really hard to never fail
this allocation. That's why we move the drm_modeset_acquire_ctx onto the
stack.

I think for understanding what's going on here you'd first need to convert
the code to the full open-code boilerplate using drm_modeset_lock_all_ctx,
with explanations of why the changes are ok. Then replace it with the
convenient macro. Once it's clear what's going on under the hood it should
then be easier to explain the situation in subsequent conversions with
just one patch.
-Daniel


> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco at gmail.com>
> ---
> 
> Changes from v1: Added further information in the commit message.
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6447cd6ca5a8..e1a71579f8e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -32,6 +32,7 @@
>  #include <linux/slab.h>
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/amdgpu_drm.h>
>  #include <linux/vgaarb.h>
> @@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>  
>  	if (!amdgpu_device_has_dc_support(adev)) {
>  		/* turn off display hw */
> -		drm_modeset_lock_all(dev);
> +		struct drm_modeset_acquire_ctx ctx;
> +		int ret;
> +
> +		DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  		drm_connector_list_iter_begin(dev, &iter);
>  		drm_for_each_connector_iter(connector, &iter)
>  			drm_helper_connector_dpms(connector,
>  						  DRM_MODE_DPMS_OFF);
>  		drm_connector_list_iter_end(&iter);
> -		drm_modeset_unlock_all(dev);
> -			/* unpin the front buffers and cursors */
> +		DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +		/* unpin the front buffers and cursors */
>  		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>  			struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  			struct drm_framebuffer *fb = crtc->primary->fb;
> @@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
>  	/* blat the mode back in */
>  	if (fbcon) {
>  		if (!amdgpu_device_has_dc_support(adev)) {
> +			struct drm_modeset_acquire_ctx ctx;
> +			int ret;
> +
>  			/* pre DCE11 */
>  			drm_helper_resume_force_mode(dev);
>  
>  			/* turn on display hw */
> -			drm_modeset_lock_all(dev);
> +			DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
>  			drm_connector_list_iter_begin(dev, &iter);
>  			drm_for_each_connector_iter(connector, &iter)
>  				drm_helper_connector_dpms(connector,
>  							  DRM_MODE_DPMS_ON);
>  			drm_connector_list_iter_end(&iter);
> -
> -			drm_modeset_unlock_all(dev);
> +			DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  		}
>  		amdgpu_fbdev_set_suspend(adev, 0);
>  	}
> -- 
> 2.31.1
> 

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