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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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);
> >  	
> >  	}