[PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers
Alex Deucher
alexdeucher at gmail.com
Fri Aug 14 19:51:28 UTC 2020
On Fri, Aug 14, 2020 at 5:38 AM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>
> This fell off in the conversion in
>
> commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
> Author: Michal Orzel <michalorzel.eng at gmail.com>
> Date: Tue Apr 28 19:10:04 2020 +0200
>
> drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
>
> but it's caught by the drm_warn_on_modeset_not_all_locked() that the
> legacy modeset code uses. Since this is the bkl and it's unclear
> what's all protected, play it safe and grab it again for legacy
> drivers.
>
> Unfortunately this means we need to sprinkle a few more #includes
> around.
>
> Also we need to add the drm_device as a parameter to the _END macro.
>
> Finally remove the mute_lock() from setcrtc, since that's now done by
> the macro.
>
> Cc: Alex Deucher <alexdeucher at gmail.com>
> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1224
> Fixes: 9bcaa3fe58ab ("drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers")
> Cc: Michal Orzel <michalorzel.eng at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Cc: <stable at vger.kernel.org> # v5.8+
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> --
> Patch compiles but otherwise untested, and I'll go on vacations now
> for 2 weeks. Alex, can you pls take care of this?
Looks good to me.
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
Also confirmed to fix the issue. I'll push to drm-misc.
Thanks!
Alex
>
> Thanks, Daniel
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 7 ++++---
> drivers/gpu/drm/drm_color_mgmt.c | 2 +-
> drivers/gpu/drm/drm_crtc.c | 4 +---
> drivers/gpu/drm/drm_mode_object.c | 4 ++--
> drivers/gpu/drm/drm_plane.c | 2 +-
> include/drm/drm_modeset_lock.h | 9 +++++++--
> 6 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f67ee513a7cc..7515a40b2056 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -34,6 +34,7 @@
> #include <drm/drm_bridge.h>
> #include <drm/drm_damage_helper.h>
> #include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
> #include <drm/drm_plane_helper.h>
> #include <drm/drm_print.h>
> #include <drm/drm_self_refresh_helper.h>
> @@ -3109,7 +3110,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
> if (ret)
> DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
>
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
> EXPORT_SYMBOL(drm_atomic_helper_shutdown);
>
> @@ -3249,7 +3250,7 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> }
>
> unlock:
> - DRM_MODESET_LOCK_ALL_END(ctx, err);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> if (err)
> return ERR_PTR(err);
>
> @@ -3330,7 +3331,7 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>
> err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
>
> - DRM_MODESET_LOCK_ALL_END(ctx, err);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> drm_atomic_state_put(state);
>
> return err;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index c93123ff7c21..138ff34b31db 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -294,7 +294,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
> crtc->gamma_size, &ctx);
>
> out:
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return ret;
>
> }
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 283bcc4362ca..aecdd7ea26dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -588,7 +588,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id))
> return -EACCES;
>
> - mutex_lock(&crtc->dev->mode_config.mutex);
> DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
>
> @@ -756,8 +755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> fb = NULL;
> mode = NULL;
>
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> - mutex_unlock(&crtc->dev->mode_config.mutex);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 901b078abf40..db05f386a709 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -428,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> out_unref:
> drm_mode_object_put(obj);
> out:
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return ret;
> }
>
> @@ -470,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> break;
> }
> drm_property_change_valid_put(prop, ref);
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index b7b90b3a2e38..affe1cfed009 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -792,7 +792,7 @@ static int setplane_internal(struct drm_plane *plane,
> crtc_x, crtc_y, crtc_w, crtc_h,
> src_x, src_y, src_w, src_h, &ctx);
>
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(plane->dev, ctx, ret);
>
> return ret;
> }
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 4fc9a43ac45a..aafd07388eb7 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -164,6 +164,8 @@ int drm_modeset_lock_all_ctx(struct drm_device *dev,
> * is 0, so no error checking is necessary
> */
> #define DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, flags, ret) \
> + if (!drm_drv_uses_atomic_modeset(dev)) \
> + mutex_lock(&dev->mode_config.mutex); \
> drm_modeset_acquire_init(&ctx, flags); \
> modeset_lock_retry: \
> ret = drm_modeset_lock_all_ctx(dev, &ctx); \
> @@ -172,6 +174,7 @@ modeset_lock_retry: \
>
> /**
> * DRM_MODESET_LOCK_ALL_END - Helper to release and cleanup modeset locks
> + * @dev: drm device
> * @ctx: local modeset acquire context, will be dereferenced
> * @ret: local ret/err/etc variable to track error status
> *
> @@ -188,7 +191,7 @@ modeset_lock_retry: \
> * to that failure. In both of these cases the code between BEGIN/END will not
> * be run, so the failure will reflect the inability to grab the locks.
> */
> -#define DRM_MODESET_LOCK_ALL_END(ctx, ret) \
> +#define DRM_MODESET_LOCK_ALL_END(dev, ctx, ret) \
> modeset_lock_fail: \
> if (ret == -EDEADLK) { \
> ret = drm_modeset_backoff(&ctx); \
> @@ -196,6 +199,8 @@ modeset_lock_fail: \
> goto modeset_lock_retry; \
> } \
> drm_modeset_drop_locks(&ctx); \
> - drm_modeset_acquire_fini(&ctx);
> + drm_modeset_acquire_fini(&ctx); \
> + if (!drm_drv_uses_atomic_modeset(dev)) \
> + mutex_unlock(&dev->mode_config.mutex);
>
> #endif /* DRM_MODESET_LOCK_H_ */
> --
> 2.28.0
>
More information about the dri-devel
mailing list