On 07/03/2024 11:27, John Ogness wrote:
On 2024-03-07, Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote:
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39ef0a6addeb..c0bb91312fb2 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -38,6 +38,7 @@
#include <drm/drm_drv.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_panic.h>
#include <drm/drm_print.h>
#include <drm/drm_self_refresh_helper.h>
#include <drm/drm_vblank.h>
@@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
}
}
+ drm_panic_lock(state->dev);
for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
WARN_ON(plane->state != old_plane_state);
@@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->planes[i].state = old_plane_state;
plane->state = new_plane_state;
}
+ drm_panic_unlock(state->dev);
Is there a reason irqsave/irqrestore variants are not used? Maybe this
code path is too hot?
This lock will be taken for each page flip, so typically at 60Hz (or
maybe 144Hz for gamers). I don't know what are the performance impacts
of the irqsave/irqrestore variant.
By leaving interrupts enabled, there is the risk that a panic from
within any interrupt handler may block the drm panic handler.
The current design is that the panic handler will just use try_lock(),
and if it can't take it, the panic screen will not be seen.
The goal is to make sure drm_panic won't crash the machine and prevent
kdump or other panic handler to run. So there is a very small race
chance that the panic screen won't be seen, but that's ok.
So I think in this case the drm panic handler shouldn't be blocked, as
it only use try_lock().
Best regards,
--
Jocelyn
John Ogness
