* [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers
@ 2020-08-14 9:38 Daniel Vetter
2020-08-14 19:51 ` Alex Deucher
0 siblings, 1 reply; 2+ messages in thread
From: Daniel Vetter @ 2020-08-14 9:38 UTC (permalink / raw)
To: DRI Development
Cc: Intel Graphics Development, Daniel Vetter, Alex Deucher,
Michal Orzel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, stable,
Daniel Vetter
This fell off in the conversion in
commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
Author: Michal Orzel <michalorzel.eng@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@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@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Daniel Vetter <daniel.vetter@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?
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers
2020-08-14 9:38 [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers Daniel Vetter
@ 2020-08-14 19:51 ` Alex Deucher
0 siblings, 0 replies; 2+ messages in thread
From: Alex Deucher @ 2020-08-14 19:51 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, Michal Orzel,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, for 3.8, Daniel Vetter
On Fri, Aug 14, 2020 at 5:38 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This fell off in the conversion in
>
> commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
> Author: Michal Orzel <michalorzel.eng@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@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@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Daniel Vetter <daniel.vetter@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@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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-08-14 19:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 9:38 [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers Daniel Vetter
2020-08-14 19:51 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).