On Tuesday, April 20, 2021 7:49:35 PM CEST Daniel Vetter wrote:

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

Thanks for the clarification. I'll go through this code again using the path you showed. Eventually I will send out another patch.

Fabio

...

Signed-off-by: Fabio M. De Francesco fmdefrancesco@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);

  }