On 17/01/2024 16:49, Thomas Zimmermann wrote:
Hi
Am 04.01.24 um 17:00 schrieb Jocelyn Falempe:
[...]
+ /**
+ * @get_scanout_buffer:
+ *
+ * Get the current scanout buffer, to display a panic message
with drm_panic.
+ * The driver should do the minimum changes to provide a linear
buffer, that
+ * can be used to display the panic screen.
+ * It is called from a panic callback, and must follow its
restrictions.
+ * (no locks, no memory allocation, no sleep, no
thread/workqueue, ...)
+ * It's a best effort mode, so it's expected that in some complex
cases the
+ * panic screen won't be displayed.
+ * Some hardware cannot provide a linear buffer, so there is a
draw_pixel_xy()
+ * callback in the struct drm_scanout_buffer that can be used in
this case.
+ *
+ * Returns:
+ *
+ * Zero on success, negative errno on failure.
+ */
+ int (*get_scanout_buffer)(struct drm_device *dev,
+ struct drm_scanout_buffer *sb);
+
After reading through Sima's comments on (try-)locking, I'd like to
propose a different interface: instead of having the panic handler
search for the scanout buffer, let each driver explicitly set the
scanout buffer after each page flip. The algorithm for mode programming
then looks like this:
1) Maybe clear the panic handler's buffer at the beginning of
atomic_commit_tail, if necessary
2) Do the mode setting as usual
3) In the driver's atomic_flush or atomic_update, call something like
void drm_panic_set_scanout_buffer(dev, scanout_buffer)
to set the panic handler's new output.
This avoids all the locking and the second guessing about the pipeline
status.
I don't see an easy way of reliably showing a panic screen during a
modeset. But during a modeset, the old scanout buffer should
(theoretically) not disappear until the new scanout buffer is in place.
So if the panic happens, it would blit to the old address at worst.
Well, that assumption needs to be verified per driver.
That's an interesting approach, and I will give it a try.
I think you still need a callback in the driver, to actually send the
data to the GPU.
Also one thing that I don't handle yet, is when there are multiple
outputs, so we may want to set and update multiple scanout buffers ?
Best regards,
--
Jocelyn
Best regards
Thomas
/** @major: driver major number */
int major;
/** @minor: driver minor number */
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
new file mode 100644
index 000000000000..bcf392b6fa1b
--- /dev/null
+++ b/include/drm/drm_panic.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+#ifndef __DRM_PANIC_H__
+#define __DRM_PANIC_H__
+
+/*
+ * Copyright (c) 2023 Red Hat.
+ * Author: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/iosys-map.h>
+
+struct drm_device;
+
+/**
+ * struct drm_scanout_buffer - DRM scanout buffer
+ *
+ * This structure holds the information necessary for drm_panic to
draw the
+ * panic screen, and display it.
+ * If the driver can't provide a linear buffer, it must clear @map with
+ * iosys_map_clear() and provide a draw_pixel_xy() function.
+ */
+struct drm_scanout_buffer {
+ /**
+ * @format:
+ *
+ * drm format of the scanout buffer.
+ */
+ const struct drm_format_info *format;
+ /**
+ * @map:
+ *
+ * Virtual address of the scanout buffer, either in memory or iomem.
+ * The scanout buffer should be in linear format, and can be
directly
+ * sent to the display hardware. Tearing is not an issue for the
panic
+ * screen.
+ */
+ struct iosys_map map;
+ /**
+ * @width: Width of the scanout buffer, in pixels.
+ */
+ unsigned int width;
+ /**
+ * @height: Height of the scanout buffer, in pixels.
+ */
+ unsigned int height;
+ /**
+ * @pitch: Length in bytes between the start of two consecutive
lines.
+ */
+ unsigned int pitch;
+ /**
+ * @private:
+ *
+ * In case the driver can't provide a linear buffer, this is a
pointer to
+ * some private data, that will be passed when calling
@draw_pixel_xy()
+ * and @flush()
+ */
+ void *private;
+ /**
+ * @draw_pixel_xy:
+ *
+ * In case the driver can't provide a linear buffer, this is a
function
+ * that drm_panic will call for each pixel to draw.
+ * Color will be converted to the format specified by @format.
+ */
+ void (*draw_pixel_xy)(unsigned int x, unsigned int y, u32 color,
void *private);
+ /**
+ * @flush:
+ *
+ * This function is called after the panic screen is drawn,
either using
+ * the iosys_map or the draw_pixel_xy path. In this function, the
driver
+ * can send additional commands to the hardware, to make the buffer
+ * visible.
+ */
+ void (*flush)(void *private);
+};
+
+#ifdef CONFIG_DRM_PANIC
+
+void drm_panic_init(void);
+void drm_panic_exit(void);
+
+void drm_panic_register(struct drm_device *dev);
+void drm_panic_unregister(struct drm_device *dev);
+
+#else
+
+static inline void drm_panic_init(void) {}
+static inline void drm_panic_exit(void) {}
+
+static inline void drm_panic_register(struct drm_device *dev) {}
+static inline void drm_panic_unregister(struct drm_device *dev) {}
+
+#endif
+
+#endif /* __DRM_PANIC_H__ */
