On Tue, Jul 20, 2021 at 03:44:49PM +0200, Daniel Vetter wrote:
On Thu, Jul 15, 2021 at 09:49:51PM +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Quite a few places are hand rolling the modeset lock backoff dance. Let's suck that into a helper macro that is easier to use without forgetting some steps.
The main downside is probably that the implementation of drm_with_modeset_lock_ctx() is a bit harder to read than a hand rolled version on account of being split across three functions, but the actual code using it ends up being much simpler.
Cc: Sean Paul seanpaul@chromium.org Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_modeset_lock.c | 44 ++++++++++++++++++++++++++++++ include/drm/drm_modeset_lock.h | 20 ++++++++++++++ 2 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index fcfe1a03c4a1..083df96632e8 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -425,3 +425,47 @@ int drm_modeset_lock_all_ctx(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_modeset_lock_all_ctx);
+void _drm_modeset_lock_begin(struct drm_modeset_acquire_ctx *ctx,
struct drm_atomic_state *state,unsigned int flags, int *ret)+{
- drm_modeset_acquire_init(ctx, flags);
- if (state)
state->acquire_ctx = ctx;- *ret = -EDEADLK;
+} +EXPORT_SYMBOL(_drm_modeset_lock_begin);
+bool _drm_modeset_lock_loop(int *ret) +{
- if (*ret == -EDEADLK) {
*ret = 0;return true;- }
- return false;
+} +EXPORT_SYMBOL(_drm_modeset_lock_loop);
+void _drm_modeset_lock_end(struct drm_modeset_acquire_ctx *ctx,
struct drm_atomic_state *state,int *ret)+{
- if (*ret == -EDEADLK) {
if (state)drm_atomic_state_clear(state);*ret = drm_modeset_backoff(ctx);if (*ret == 0) {*ret = -EDEADLK;return;}- }
- drm_modeset_drop_locks(ctx);
- drm_modeset_acquire_fini(ctx);
+} +EXPORT_SYMBOL(_drm_modeset_lock_end); diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index aafd07388eb7..5eaad2533de5 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -26,6 +26,7 @@
#include <linux/ww_mutex.h>
+struct drm_atomic_state; struct drm_modeset_lock;
/** @@ -203,4 +204,23 @@ modeset_lock_fail: \ if (!drm_drv_uses_atomic_modeset(dev)) \ mutex_unlock(&dev->mode_config.mutex);
+void _drm_modeset_lock_begin(struct drm_modeset_acquire_ctx *ctx,
struct drm_atomic_state *state,unsigned int flags,int *ret);+bool _drm_modeset_lock_loop(int *ret); +void _drm_modeset_lock_end(struct drm_modeset_acquire_ctx *ctx,
struct drm_atomic_state *state,int *ret);+/*
- Note that one must always use "continue" rather than
- "break" or "return" to handle errors within the
- drm_modeset_lock_ctx_retry() block.
I'm not sold on loop macros with these kind of restrictions, C just isn't a great language for these. That's why e.g. drm_connector_iter doesn't give you a macro, but only the begin/next/end function calls explicitly.
We already use this pattern extensively in i915. Gem ww ctx has one, power domains/pps/etc. use a similar things. It makes the code pretty nice, with the slight caveat that an accidental 'break' can ruin your day. But so can an accidental return with other constructs (and we even had that happen a few times with the connector iterators), so not a dealbreaker IMO.
So if we don't want this drm wide I guess I can propose this just for i915 since it fits in perfectly there.
Yes the macro we have is also not nice, but at least it's a screaming macro since it's all uppercase, so options are all a bit sucky. Which leads me to think we have a bit a https://xkcd.com/927/ situation going on.
I think minimally we should have one way to do this.
Well, there is no one way atm. All you can do is hand roll all the boilerplate (and likely get it slightly wrong) if you don't want lock_all.
The current macros only help with lock_all, and IMO the hidden gotos are even uglier than a hidden for loop. Fernando already hit a case where he couldn't use the macros twice due to conflicting goto labels. With this for loop thing I think it would have just worked(tm).