On 07/03/2024 11:27, John Ogness wrote:
On 2024-03-07, Jocelyn Falempe <[email protected]> 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
