Re: [PATCH v3 12/14] drm/panic: Internalize panic locking in DRM core and helpers
On 9/2/26 14:13, Thomas Zimmermann wrote:
None of the DRM drivers handles panic locking. Declare the interfaces
in an internal header file. Move the trylock required during the panic
into the DRM core's sources.
Thanks, it looks good to me.
Reviewed-by: Jocelyn Falempe
Signed-off-by: Thomas Zimmermann
---
Documentation/gpu/drm-kms.rst| 3 ++
MAINTAINERS | 3 +-
drivers/gpu/drm/drm_atomic_helper.c | 2 +-
drivers/gpu/drm/drm_drv.c| 2 +-
drivers/gpu/drm/drm_panic.c | 4 ++
drivers/gpu/drm/drm_panic_internal.h | 55 +++
include/drm/drm_panic.h | 65
7 files changed, 66 insertions(+), 68 deletions(-)
create mode 100644 drivers/gpu/drm/drm_panic_internal.h
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 0dd440a14946..8988fd64369b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -421,6 +421,9 @@ Plane Panic Functions Reference
.. kernel-doc:: include/drm/drm_panic.h
:internal:
+.. kernel-doc:: drivers/gpu/drm/drm_panic_internal.h
+ :internal:
+
.. kernel-doc:: drivers/gpu/drm/drm_panic.c
:export:
diff --git a/MAINTAINERS b/MAINTAINERS
index c66c3fe93f3d..ccc5dc0a01ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9111,7 +9111,8 @@ S:Supported
T:git https://gitlab.freedesktop.org/drm/misc/kernel.git
F:drivers/gpu/drm/drm_draw.c
F:drivers/gpu/drm/drm_draw_internal.h
-F: drivers/gpu/drm/drm_panic*.c
+F: drivers/gpu/drm/drm_panic.c
+F: drivers/gpu/drm/drm_panic_internal.h
F:drivers/gpu/drm/tests/drm_panic_test.c
F:include/drm/drm_panic.h
F:include/drm/drm_panic_helper.h
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 285aac3554df..af3934df3313 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -41,7 +41,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -49,6 +48,7 @@
#include "drm_crtc_helper_internal.h"
#include "drm_crtc_internal.h"
+#include "drm_panic_internal.h"
/**
* DOC: overview
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8c0879c336e5..64055514acbf 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -51,13 +51,13 @@
#include
#include
#include
-#include
#include
#include
#include
#include "drm_crtc_internal.h"
#include "drm_internal.h"
+#include "drm_panic_internal.h"
MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl");
MODULE_DESCRIPTION("DRM shared core routines");
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 7b9864857577..41ba0c4b050e 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -35,6 +35,7 @@
#include "drm_crtc_internal.h"
#include "drm_draw_internal.h"
+#include "drm_panic_internal.h"
MODULE_AUTHOR("Jocelyn Falempe");
MODULE_DESCRIPTION("DRM panic handler");
@@ -976,6 +977,9 @@ int drm_plane_helper_display_panic_screen(struct drm_plane
*plane, const char *d
}
EXPORT_SYMBOL(drm_plane_helper_display_panic_screen);
+#define drm_panic_trylock(dev, flags) \
+ raw_spin_trylock_irqsave(&(dev)->mode_config.panic_lock, flags)
+
static void drm_panic_display_panic_screen(struct drm_plane *plane, const
char *description)
{
#if defined(CONFIG_DRM_PANIC_FOREGROUND_COLOR)
diff --git a/drivers/gpu/drm/drm_panic_internal.h
b/drivers/gpu/drm/drm_panic_internal.h
new file mode 100644
index ..c68d64c56e15
--- /dev/null
+++ b/drivers/gpu/drm/drm_panic_internal.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+
+/*
+ * Copyright (c) 2024 Intel
+ * Copyright (c) 2024 Red Hat
+ */
+
+#ifndef __DRM_PANIC_INTERNAL_H__
+#define __DRM_PANIC_INTERNAL_H__
+
+#include
+
+struct drm_device;
+
+#ifdef CONFIG_DRM_PANIC
+
+/**
+ * drm_panic_lock - protect panic printing relevant state
+ * @dev: struct drm_device
+ * @flags: unsigned long irq flags you need to pass to the unlock() counterpart
+ *
+ * This function must be called to protect software and hardware state that the
+ * panic printing code must be able to rely on. The protected sections must be
+ * as small as possible. It uses the irqsave/irqrestore variant, and can be
+ * called from irq handler. Examples include:
+ *
+ * - Access to peek/poke or other similar registers, if that is the way the
+ * driver prints the pixels into the scanout buffer at panic time.
+ *
+ * - Updates to pointers like &drm_plane.state, allowing the panic handler to
+ * safely deference these. This is done in drm_atomic_helper_swap_state().
+ *
+ * - An state that isn't invariant and that the driver must be able to access
+ * during panic printing.
+ */
+#define drm_panic_lock(dev, flags) \
+
[PATCH v3 12/14] drm/panic: Internalize panic locking in DRM core and helpers
None of the DRM drivers handles panic locking. Declare the interfaces
in an internal header file. Move the trylock required during the panic
into the DRM core's sources.
Signed-off-by: Thomas Zimmermann
---
Documentation/gpu/drm-kms.rst| 3 ++
MAINTAINERS | 3 +-
drivers/gpu/drm/drm_atomic_helper.c | 2 +-
drivers/gpu/drm/drm_drv.c| 2 +-
drivers/gpu/drm/drm_panic.c | 4 ++
drivers/gpu/drm/drm_panic_internal.h | 55 +++
include/drm/drm_panic.h | 65
7 files changed, 66 insertions(+), 68 deletions(-)
create mode 100644 drivers/gpu/drm/drm_panic_internal.h
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 0dd440a14946..8988fd64369b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -421,6 +421,9 @@ Plane Panic Functions Reference
.. kernel-doc:: include/drm/drm_panic.h
:internal:
+.. kernel-doc:: drivers/gpu/drm/drm_panic_internal.h
+ :internal:
+
.. kernel-doc:: drivers/gpu/drm/drm_panic.c
:export:
diff --git a/MAINTAINERS b/MAINTAINERS
index c66c3fe93f3d..ccc5dc0a01ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9111,7 +9111,8 @@ S:Supported
T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
F: drivers/gpu/drm/drm_draw.c
F: drivers/gpu/drm/drm_draw_internal.h
-F: drivers/gpu/drm/drm_panic*.c
+F: drivers/gpu/drm/drm_panic.c
+F: drivers/gpu/drm/drm_panic_internal.h
F: drivers/gpu/drm/tests/drm_panic_test.c
F: include/drm/drm_panic.h
F: include/drm/drm_panic_helper.h
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 285aac3554df..af3934df3313 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -41,7 +41,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -49,6 +48,7 @@
#include "drm_crtc_helper_internal.h"
#include "drm_crtc_internal.h"
+#include "drm_panic_internal.h"
/**
* DOC: overview
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8c0879c336e5..64055514acbf 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -51,13 +51,13 @@
#include
#include
#include
-#include
#include
#include
#include
#include "drm_crtc_internal.h"
#include "drm_internal.h"
+#include "drm_panic_internal.h"
MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl");
MODULE_DESCRIPTION("DRM shared core routines");
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 7b9864857577..41ba0c4b050e 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -35,6 +35,7 @@
#include "drm_crtc_internal.h"
#include "drm_draw_internal.h"
+#include "drm_panic_internal.h"
MODULE_AUTHOR("Jocelyn Falempe");
MODULE_DESCRIPTION("DRM panic handler");
@@ -976,6 +977,9 @@ int drm_plane_helper_display_panic_screen(struct drm_plane
*plane, const char *d
}
EXPORT_SYMBOL(drm_plane_helper_display_panic_screen);
+#define drm_panic_trylock(dev, flags) \
+ raw_spin_trylock_irqsave(&(dev)->mode_config.panic_lock, flags)
+
static void drm_panic_display_panic_screen(struct drm_plane *plane, const char
*description)
{
#if defined(CONFIG_DRM_PANIC_FOREGROUND_COLOR)
diff --git a/drivers/gpu/drm/drm_panic_internal.h
b/drivers/gpu/drm/drm_panic_internal.h
new file mode 100644
index ..c68d64c56e15
--- /dev/null
+++ b/drivers/gpu/drm/drm_panic_internal.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+
+/*
+ * Copyright (c) 2024 Intel
+ * Copyright (c) 2024 Red Hat
+ */
+
+#ifndef __DRM_PANIC_INTERNAL_H__
+#define __DRM_PANIC_INTERNAL_H__
+
+#include
+
+struct drm_device;
+
+#ifdef CONFIG_DRM_PANIC
+
+/**
+ * drm_panic_lock - protect panic printing relevant state
+ * @dev: struct drm_device
+ * @flags: unsigned long irq flags you need to pass to the unlock() counterpart
+ *
+ * This function must be called to protect software and hardware state that the
+ * panic printing code must be able to rely on. The protected sections must be
+ * as small as possible. It uses the irqsave/irqrestore variant, and can be
+ * called from irq handler. Examples include:
+ *
+ * - Access to peek/poke or other similar registers, if that is the way the
+ * driver prints the pixels into the scanout buffer at panic time.
+ *
+ * - Updates to pointers like &drm_plane.state, allowing the panic handler to
+ * safely deference these. This is done in drm_atomic_helper_swap_state().
+ *
+ * - An state that isn't invariant and that the driver must be able to access
+ * during panic printing.
+ */
+#define drm_panic_lock(dev, flags) \
+ raw_spin_lock_irqsave(&(dev)->mode_config.panic_lock, flags)
+
+/**
+ * drm_panic_unlock - end of the panic printing critical section
+ * @dev: struct drm_device
+ * @flags: irq flags that were
