[PATCH 0/4] drm: Make modeset locking easier

[PATCH 0/4] drm: Make modeset locking easier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

While staring at some DRM_MODESET_LOCK_ALL_{BEGIN,END}() conversions
I decided I don't really like what those macros do.

The main problems that I see:
- the magic goto inside limits their usefulness to baically
  a single statement, unless you're willing to look inside and
  find out the name of the magic label
- can't help at all in the "we don't want to lock everything"
  cases, which are quite numerous
- not at all obvious that there's a loop in there

So I figured I'd try to come up with something more universally useful.

Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel@ffwll.ch>

Ville Syrjälä (4):
  drm: Introduce drm_modeset_lock_ctx_retry()
  drm: Introduce drm_modeset_lock_all_ctx_retry()
  drm/i915: Extract intel_crtc_initial_commit()
  drm/i915: Use drm_modeset_lock_ctx_retry() & co.

 drivers/gpu/drm/drm_modeset_lock.c            |  44 ++++
 drivers/gpu/drm/i915/display/g4x_dp.c         |  17 +-
 drivers/gpu/drm/i915/display/intel_audio.c    |  17 +-
 drivers/gpu/drm/i915/display/intel_ddi.c      |  16 +-
 drivers/gpu/drm/i915/display/intel_display.c  | 192 ++++++++----------
 .../drm/i915/display/intel_display_debugfs.c  |  45 ++--
 drivers/gpu/drm/i915/display/intel_pipe_crc.c |  38 ++--
 include/drm/drm_modeset_lock.h                |  26 +++
 8 files changed, 188 insertions(+), 207 deletions(-)

-- 
2.31.1

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

[Ville Syrjala]

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.
+ */
+#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

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

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

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

[Daniel Vetter]

On Mon, Oct 04, 2021 at 02:15:51PM +0300, Ville Syrjälä wrote:
> 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.

Well I don't like them for i915 either.

And yes C is dangerous, but also C is verbose. I think one lesson from igt
is that too many magic block constructs are bad, it's just not how C
works. Definitely not in the kernel, where "oops I got it wrong because it
was too clever" is bad.

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

I'm totally ok with repainting the shed, I just don't want some 80s
multicolor flash show.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Wed, Oct 13, 2021 at 01:59:47PM +0200, Daniel Vetter wrote:
> On Mon, Oct 04, 2021 at 02:15:51PM +0300, Ville Syrjälä wrote:
> > 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.
> 
> Well I don't like them for i915 either.

I think you're a bit alone in that. Most people seem pretty happy
with this style since it makes the code very readable.

> 
> And yes C is dangerous, but also C is verbose. I think one lesson from igt
> is that too many magic block constructs are bad, it's just not how C
> works. Definitely not in the kernel, where "oops I got it wrong because it
> was too clever" is bad.
> 
> > > 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).
> 
> I'm totally ok with repainting the shed, I just don't want some 80s
> multicolor flash show.

You have a better idea in mind?

-- 
Ville Syrjälä
Intel

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

[Fernando Ramos]

On 21/10/13 03:06PM, Ville Syrjälä wrote:
> > And yes C is dangerous, but also C is verbose. I think one lesson from igt
> > is that too many magic block constructs are bad, it's just not how C
> > works. Definitely not in the kernel, where "oops I got it wrong because it
> > was too clever" is bad.
> > 
> > > > 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).
> > 
> > I'm totally ok with repainting the shed, I just don't want some 80s
> > multicolor flash show.
> 
> You have a better idea in mind?

Sorry, I completely forgot this discussion was going on and I just published V4
of my patch set here:

    https://lore.kernel.org/dri-devel/20211013204846.90026-1-greenfoo@u92.eu/

Please, feel free to let me know (ideally, as a reply to the corresponding i915
patch from that set) if you rather me not to modify i915 files for now.

Thanks.

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

[Daniel Vetter]

On Wed, Oct 13, 2021 at 11:00:35PM +0200, Fernando Ramos wrote:
> On 21/10/13 03:06PM, Ville Syrjälä wrote:
> > > And yes C is dangerous, but also C is verbose. I think one lesson from igt
> > > is that too many magic block constructs are bad, it's just not how C
> > > works. Definitely not in the kernel, where "oops I got it wrong because it
> > > was too clever" is bad.
> > > 
> > > > > 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).
> > > 
> > > I'm totally ok with repainting the shed, I just don't want some 80s
> > > multicolor flash show.
> > 
> > You have a better idea in mind?
> 
> Sorry, I completely forgot this discussion was going on and I just published V4
> of my patch set here:
> 
>     https://lore.kernel.org/dri-devel/20211013204846.90026-1-greenfoo@u92.eu/
> 
> Please, feel free to let me know (ideally, as a reply to the corresponding i915
> patch from that set) if you rather me not to modify i915 files for now.

My request is that we only have one way of doing this drm_modeset lock
retry business. So either this one here proposed by Ville, or the one Sean
Paul merged.

I honestly don't care which color we pick, as long as it's consistent
across the board. Please all you, figure this out.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/4] drm: Introduce drm_modeset_lock_all_ctx_retry()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Layer drm_modeset_lock_all_ctx_retry() on top of
drm_modeset_lock_ctx_retry() to make the fairly common
"let's lock everything" pattern nicer.

Currently we have DRM_MODESET_LOCK_ALL_{BEGIN,END}() for this
but I don't really like it due to the magic gotos within,
which makes it hard to use if you want to do multiple steps
between the BEGING/END. One would either have to know the
name of the magic label, or always wrap the whole thing into
a function so only the single call appears between the BEGIN/END.

Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modeset_lock.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index 5eaad2533de5..2e2548680aaa 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -223,4 +223,10 @@ void _drm_modeset_lock_end(struct drm_modeset_acquire_ctx *ctx,
 	     _drm_modeset_lock_loop(&(ret)); \
 	     _drm_modeset_lock_end((ctx), (state), &(ret)))
 
+#define drm_modeset_lock_all_ctx_retry(dev, 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))) \
+		for_each_if(((ret) = drm_modeset_lock_all_ctx((dev), (ctx))) == 0)
+
 #endif /* DRM_MODESET_LOCK_H_ */
-- 
2.31.1

[PATCH 3/4] drm/i915: Extract intel_crtc_initial_commit()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Extract intel_crtc_initial_commit() from intel_initial_commit().
Should make subsequent changes a bit less convoluted.

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/i915/display/intel_display.c | 96 +++++++++++---------
 1 file changed, 52 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 65ddb6ca16e6..3718399c4c2f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12129,12 +12129,60 @@ static void intel_update_fdi_pll_freq(struct drm_i915_private *dev_priv)
 	drm_dbg(&dev_priv->drm, "FDI PLL freq=%d\n", dev_priv->fdi_pll_freq);
 }
 
+static int intel_crtc_initial_commit(struct intel_atomic_state *state,
+				     struct intel_crtc *crtc)
+{
+	struct intel_crtc_state *crtc_state;
+	struct intel_encoder *encoder;
+	int ret;
+
+	crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	if (!crtc_state->hw.active)
+		return 0;
+
+	/*
+	 * We've not yet detected sink capabilities
+	 * (audio,infoframes,etc.) and thus we don't want to
+	 * force a full state recomputation yet. We want that to
+	 * happen only for the first real commit from userspace.
+	 * So preserve the inherited flag for the time being.
+	 */
+	crtc_state->inherited = true;
+
+	ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
+	if (ret)
+		return ret;
+
+	/*
+	 * FIXME hack to force a LUT update to avoid the
+	 * plane update forcing the pipe gamma on without
+	 * having a proper LUT loaded. Remove once we
+	 * have readout for pipe gamma enable.
+	 */
+	crtc_state->uapi.color_mgmt_changed = true;
+
+	for_each_intel_encoder_mask(state->base.dev, encoder, crtc_state->uapi.encoder_mask) {
+		if (encoder->initial_fastset_check &&
+		    !encoder->initial_fastset_check(encoder, crtc_state)) {
+			ret = drm_atomic_add_affected_connectors(&state->base,
+								 &crtc->base);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int intel_initial_commit(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = NULL;
 	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
 	struct intel_crtc *crtc;
-	int ret = 0;
+	int ret;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -12146,49 +12194,9 @@ static int intel_initial_commit(struct drm_device *dev)
 	state->acquire_ctx = &ctx;
 
 	for_each_intel_crtc(dev, crtc) {
-		struct intel_crtc_state *crtc_state =
-			intel_atomic_get_crtc_state(state, crtc);
-
-		if (IS_ERR(crtc_state)) {
-			ret = PTR_ERR(crtc_state);
+		ret = intel_crtc_initial_commit(to_intel_atomic_state(state), crtc);
+		if (ret)
 			goto out;
-		}
-
-		if (crtc_state->hw.active) {
-			struct intel_encoder *encoder;
-
-			/*
-			 * We've not yet detected sink capabilities
-			 * (audio,infoframes,etc.) and thus we don't want to
-			 * force a full state recomputation yet. We want that to
-			 * happen only for the first real commit from userspace.
-			 * So preserve the inherited flag for the time being.
-			 */
-			crtc_state->inherited = true;
-
-			ret = drm_atomic_add_affected_planes(state, &crtc->base);
-			if (ret)
-				goto out;
-
-			/*
-			 * FIXME hack to force a LUT update to avoid the
-			 * plane update forcing the pipe gamma on without
-			 * having a proper LUT loaded. Remove once we
-			 * have readout for pipe gamma enable.
-			 */
-			crtc_state->uapi.color_mgmt_changed = true;
-
-			for_each_intel_encoder_mask(dev, encoder,
-						    crtc_state->uapi.encoder_mask) {
-				if (encoder->initial_fastset_check &&
-				    !encoder->initial_fastset_check(encoder, crtc_state)) {
-					ret = drm_atomic_add_affected_connectors(state,
-										 &crtc->base);
-					if (ret)
-						goto out;
-				}
-			}
-		}
 	}
 
 	ret = drm_atomic_commit(state);
-- 
2.31.1

[PATCH 4/4] drm/i915: Use drm_modeset_lock_ctx_retry() & co.

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have the modeset lock dance hand rolled in quite a few places.
Use drm_modeset_lock_ctx_retry() and drm_modeset_lock_all_ctx_retry()
to simplify our lives.

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/i915/display/g4x_dp.c         |  17 +--
 drivers/gpu/drm/i915/display/intel_audio.c    |  17 +--
 drivers/gpu/drm/i915/display/intel_ddi.c      |  16 +--
 drivers/gpu/drm/i915/display/intel_display.c  | 102 ++++++------------
 .../drm/i915/display/intel_display_debugfs.c  |  45 +++-----
 drivers/gpu/drm/i915/display/intel_pipe_crc.c |  38 +++----
 6 files changed, 69 insertions(+), 166 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index de0f358184aa..68e7f4233fa9 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -1167,23 +1167,10 @@ intel_dp_hotplug(struct intel_encoder *encoder,
 
 	state = intel_encoder_hotplug(encoder, connector);
 
-	drm_modeset_acquire_init(&ctx, 0);
-
-	for (;;) {
+	drm_modeset_lock_ctx_retry(&ctx, NULL, 0, ret)
 		ret = intel_dp_retrain_link(encoder, &ctx);
 
-		if (ret == -EDEADLK) {
-			drm_modeset_backoff(&ctx);
-			continue;
-		}
-
-		break;
-	}
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	drm_WARN(encoder->base.dev, ret,
-		 "Acquiring modeset locks failed with %i\n", ret);
+	drm_WARN_ON(encoder->base.dev, ret);
 
 	/*
 	 * Keeping it consistent with intel_ddi_hotplug() and
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 5f4f316b3ab5..a3b6b00e6633 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -969,28 +969,17 @@ static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
 	if (!crtc)
 		return;
 
-	drm_modeset_acquire_init(&ctx, 0);
 	state = drm_atomic_state_alloc(&dev_priv->drm);
 	if (drm_WARN_ON(&dev_priv->drm, !state))
 		return;
 
-	state->acquire_ctx = &ctx;
-
-retry:
-	ret = glk_force_audio_cdclk_commit(to_intel_atomic_state(state), crtc,
-					   enable);
-	if (ret == -EDEADLK) {
-		drm_atomic_state_clear(state);
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
+	drm_modeset_lock_ctx_retry(&ctx, state, 0, ret)
+		ret = glk_force_audio_cdclk_commit(to_intel_atomic_state(state),
+						   crtc, enable);
 
 	drm_WARN_ON(&dev_priv->drm, ret);
 
 	drm_atomic_state_put(state);
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static unsigned long i915_audio_component_get_power(struct device *kdev)
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 26a3aa73fcc4..9aa7369d8dbe 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4210,26 +4210,14 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
 
 	state = intel_encoder_hotplug(encoder, connector);
 
-	drm_modeset_acquire_init(&ctx, 0);
-
-	for (;;) {
+	drm_modeset_lock_ctx_retry(&ctx, NULL, 0, ret) {
 		if (connector->base.connector_type == DRM_MODE_CONNECTOR_HDMIA)
 			ret = intel_hdmi_reset_link(encoder, &ctx);
 		else
 			ret = intel_dp_retrain_link(encoder, &ctx);
-
-		if (ret == -EDEADLK) {
-			drm_modeset_backoff(&ctx);
-			continue;
-		}
-
-		break;
 	}
 
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	drm_WARN(encoder->base.dev, ret,
-		 "Acquiring modeset locks failed with %i\n", ret);
+	drm_WARN_ON(encoder->base.dev, ret);
 
 	/*
 	 * Unpowered type-c dongles can take some time to boot and be
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3718399c4c2f..6f5bc56d68e0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12057,40 +12057,30 @@ static void sanitize_watermarks(struct drm_i915_private *dev_priv)
 
 	intel_state = to_intel_atomic_state(state);
 
-	drm_modeset_acquire_init(&ctx, 0);
+	drm_modeset_lock_ctx_retry(&ctx, state, 0, ret) {
+		/*
+		 * Hardware readout is the only time we don't want to calculate
+		 * intermediate watermarks (since we don't trust the current
+		 * watermarks).
+		 */
+		if (!HAS_GMCH(dev_priv))
+			intel_state->skip_intermediate_wm = true;
 
-retry:
-	state->acquire_ctx = &ctx;
+		ret = sanitize_watermarks_add_affected(state);
+		if (ret)
+			continue;
 
-	/*
-	 * Hardware readout is the only time we don't want to calculate
-	 * intermediate watermarks (since we don't trust the current
-	 * watermarks).
-	 */
-	if (!HAS_GMCH(dev_priv))
-		intel_state->skip_intermediate_wm = true;
+		ret = intel_atomic_check(&dev_priv->drm, state);
+		if (ret)
+			continue;
 
-	ret = sanitize_watermarks_add_affected(state);
-	if (ret)
-		goto fail;
+		/* Write calculated watermark values back */
+		for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
+			crtc_state->wm.need_postvbl_update = true;
+			dev_priv->display.optimize_watermarks(intel_state, crtc);
 
-	ret = intel_atomic_check(&dev_priv->drm, state);
-	if (ret)
-		goto fail;
-
-	/* Write calculated watermark values back */
-	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
-		crtc_state->wm.need_postvbl_update = true;
-		dev_priv->display.optimize_watermarks(intel_state, crtc);
-
-		to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm;
-	}
-
-fail:
-	if (ret == -EDEADLK) {
-		drm_atomic_state_clear(state);
-		drm_modeset_backoff(&ctx);
-		goto retry;
+			to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm;
+		}
 	}
 
 	/*
@@ -12108,9 +12098,6 @@ static void sanitize_watermarks(struct drm_i915_private *dev_priv)
 		 "Could not determine valid watermarks for inherited state\n");
 
 	drm_atomic_state_put(state);
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static void intel_update_fdi_pll_freq(struct drm_i915_private *dev_priv)
@@ -12181,38 +12168,28 @@ static int intel_initial_commit(struct drm_device *dev)
 {
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_atomic_state *state;
-	struct intel_crtc *crtc;
 	int ret;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
 		return -ENOMEM;
 
-	drm_modeset_acquire_init(&ctx, 0);
+	drm_modeset_lock_ctx_retry(&ctx, state, 0, ret) {
+		struct intel_crtc *crtc;
 
-retry:
-	state->acquire_ctx = &ctx;
-
-	for_each_intel_crtc(dev, crtc) {
-		ret = intel_crtc_initial_commit(to_intel_atomic_state(state), crtc);
+		for_each_intel_crtc(dev, crtc) {
+			ret = intel_crtc_initial_commit(to_intel_atomic_state(state), crtc);
+			if (ret)
+				break;
+		}
 		if (ret)
-			goto out;
-	}
+			continue;
 
-	ret = drm_atomic_commit(state);
-
-out:
-	if (ret == -EDEADLK) {
-		drm_atomic_state_clear(state);
-		drm_modeset_backoff(&ctx);
-		goto retry;
+		ret = drm_atomic_commit(state);
 	}
 
 	drm_atomic_state_put(state);
 
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-
 	return ret;
 }
 
@@ -13323,25 +13300,14 @@ void intel_display_resume(struct drm_device *dev)
 		return;
 
 	dev_priv->modeset_restore_state = NULL;
-	if (state)
-		state->acquire_ctx = &ctx;
 
-	drm_modeset_acquire_init(&ctx, 0);
-
-	while (1) {
-		ret = drm_modeset_lock_all_ctx(dev, &ctx);
-		if (ret != -EDEADLK)
-			break;
-
-		drm_modeset_backoff(&ctx);
-	}
-
-	if (!ret)
+	drm_modeset_lock_all_ctx_retry(dev, &ctx, state, 0, ret) {
 		ret = __intel_display_resume(dev, state, &ctx);
+		if (ret)
+			continue;
 
-	intel_enable_ipc(dev_priv);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
+		intel_enable_ipc(dev_priv);
+	}
 
 	if (ret)
 		drm_err(&dev_priv->drm,
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 65832c4d962f..e523a0ec6534 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -2294,44 +2294,32 @@ static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
 {
 	struct drm_connector *connector = m->private;
 	struct drm_device *dev = connector->dev;
-	struct drm_crtc *crtc;
-	struct intel_dp *intel_dp;
 	struct drm_modeset_acquire_ctx ctx;
-	struct intel_crtc_state *crtc_state = NULL;
-	int ret = 0;
-	bool try_again = false;
+	int ret;
 
-	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+	drm_modeset_lock_ctx_retry(&ctx, NULL, DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret) {
+		struct intel_crtc_state *crtc_state;
+		struct intel_dp *intel_dp;
+		struct drm_crtc *crtc;
 
-	do {
-		try_again = false;
 		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
 				       &ctx);
-		if (ret) {
-			if (ret == -EDEADLK && !drm_modeset_backoff(&ctx)) {
-				try_again = true;
-				continue;
-			}
-			break;
-		}
+		if (ret)
+			continue;
+
 		crtc = connector->state->crtc;
 		if (connector->status != connector_status_connected || !crtc) {
 			ret = -ENODEV;
-			break;
+			continue;
 		}
+
 		ret = drm_modeset_lock(&crtc->mutex, &ctx);
-		if (ret == -EDEADLK) {
-			ret = drm_modeset_backoff(&ctx);
-			if (!ret) {
-				try_again = true;
-				continue;
-			}
-			break;
-		} else if (ret) {
-			break;
-		}
+		if (ret)
+			continue;
+
 		intel_dp = intel_attached_dp(to_intel_connector(connector));
 		crtc_state = to_intel_crtc_state(crtc->state);
+
 		seq_printf(m, "DSC_Enabled: %s\n",
 			   yesno(crtc_state->dsc.compression_enable));
 		seq_printf(m, "DSC_Sink_Support: %s\n",
@@ -2341,10 +2329,7 @@ static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
 		if (!intel_dp_is_edp(intel_dp))
 			seq_printf(m, "FEC_Sink_Support: %s\n",
 				   yesno(drm_dp_sink_supports_fec(intel_dp->fec_capable)));
-	} while (try_again);
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
+	}
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
index 8ac263f471be..89435de4ff58 100644
--- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
@@ -293,46 +293,34 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 
-	drm_modeset_acquire_init(&ctx, 0);
-
 	state = drm_atomic_state_alloc(&dev_priv->drm);
 	if (!state) {
 		ret = -ENOMEM;
 		goto unlock;
 	}
 
-	state->acquire_ctx = &ctx;
+	drm_modeset_lock_ctx_retry(&ctx, state, 0, ret) {
+		pipe_config = intel_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(pipe_config)) {
+			ret = PTR_ERR(pipe_config);
+			continue;
+		}
 
-retry:
-	pipe_config = intel_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(pipe_config)) {
-		ret = PTR_ERR(pipe_config);
-		goto put_state;
-	}
+		pipe_config->uapi.mode_changed = pipe_config->has_psr;
+		pipe_config->crc_enabled = enable;
 
-	pipe_config->uapi.mode_changed = pipe_config->has_psr;
-	pipe_config->crc_enabled = enable;
+		if (IS_HASWELL(dev_priv) &&
+		    pipe_config->hw.active && crtc->pipe == PIPE_A &&
+		    pipe_config->cpu_transcoder == TRANSCODER_EDP)
+			pipe_config->uapi.mode_changed = true;
 
-	if (IS_HASWELL(dev_priv) &&
-	    pipe_config->hw.active && crtc->pipe == PIPE_A &&
-	    pipe_config->cpu_transcoder == TRANSCODER_EDP)
-		pipe_config->uapi.mode_changed = true;
-
-	ret = drm_atomic_commit(state);
-
-put_state:
-	if (ret == -EDEADLK) {
-		drm_atomic_state_clear(state);
-		drm_modeset_backoff(&ctx);
-		goto retry;
+		ret = drm_atomic_commit(state);
 	}
 
 	drm_atomic_state_put(state);
 unlock:
 	drm_WARN(&dev_priv->drm, ret,
 		 "Toggling workaround to %i returns %i\n", enable, ret);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
-- 
2.31.1