| Message ID | 20210419150341.24959-2-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_* | expand |
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@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 >
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); > > > > }
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); }
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. 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(-)