Re: [PATCH 1/4] drm: Introduce drm_modeset_lock_ctx_retry()

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.

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. -Daniel

• */

+#define drm_modeset_lock_ctx_retry(ctx, state, flags, ret) \

• for (_drm_modeset_lock_begin((ctx), (state), (flags), &(ret)); \

•     _drm_modeset_lock_loop(&(ret)); \
  

•     _drm_modeset_lock_end((ctx), (state), &(ret)))
  

#endif /* DRM_MODESET_LOCK_H_ */

2.31.1