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(-)
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_ */
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
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).
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
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?
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.
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
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_ */
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);
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,
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Fernando Ramos -
Ville Syrjala -
Ville Syrjälä