On 06/10/2023 18:54, Noralf Trønnes wrote:
On 10/6/23 16:35, Maxime Ripard wrote:
Hi Jocelyn,
On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
On 05/10/2023 10:18, Maxime Ripard wrote:
Hi,
On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 89e2706cac56..e538c87116d3 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -43,6 +43,7 @@ struct dma_buf_attachment;
struct drm_display_mode;
struct drm_mode_create_dumb;
struct drm_printer;
+struct drm_scanout_buffer;
struct sg_table;
/**
@@ -408,6 +409,19 @@ struct drm_driver {
*/
void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
+ /**
+ * @get_scanout_buffer:
+ *
+ * Get the current scanout buffer, to display a panic message with drm_panic.
+ * It is called from a panic callback, and must follow its restrictions.
+ *
+ * Returns:
+ *
+ * Zero on success, negative errno on failure.
+ */
+ int (*get_scanout_buffer)(struct drm_device *dev,
+ struct drm_scanout_buffer *sb);
+
What is the format of that buffer? What is supposed to happen if the
planes / CRTC are setup in a way that is incompatible with the buffer
format?
Currently, it only supports linear format, either in system memory, or
iomem.
But really what is needed is the screen size, and a way to write pixels to
it.
For more complex GPU, I don't know if it's easier to reprogram the GPU to
linear format, or to add a simple "tiled" support to drm_panic.
What would you propose as a panic interface to handle those complex format ?
It's not just about tiling, but also about YUV formats. If the display
engine is currently playing a video at the moment, it's probably going
to output some variation of multi-planar YUV and you won't have an RGB
buffer available.
I had support for some YUV formats in my 2019 attempt on a panic
handler[1] and I made a recording of a test run as well[2] (see 4:30 for
YUV). There was a discussion about challenges and i915 can disable
tiling by flipping a bit in a register[3] and AMD has a debug
interface[4] they can use to write pixels.
I only added support for the format used by simpledrm, because I don't
want to add support for all possible format if no driver are using it.
It should be possible to add YUV format too.
I also prefer to convert only the foreground/background color, and then
write directly into the buffers, instead of converting line by line.
It works for all format where pixel size is a multiple of byte.
Noralf.
[1]
https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@xxxxxxxxxxx/
[2] https://youtu.be/lZ80vL4dgpE
[3]
https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
[4]
https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@xxxxxxx/
Same story if you're using a dma-buf buffer. You might not even be able
to access that buffer at all from the CPU or the kernel.
I really think we should have some emergency state ready to commit on
the side, and possibly a panic_commit function to prevent things like
sleeping or waiting that regular atomic_commit can use.
That way, you know have all the resources available to you any time.
I think reusing the atomic commit functions might be hard, because there
are locks/allocation/threads hidden in drivers callback. I'm more in
favor of an emergency function, that each driver has to implement, and
use what the hardware can do to display a simple frame quickly.
get_scanout_buffer() is a good start for simple driver, but will need
refactoring for the more complex case, like adding a callback to write
pixels one by one, if there is no memory mapped buffer available.
Sometime it's also just not possible to write pixels to the screen, like if
the panic occurs in the middle of suspend/resume, or during a mode-setting,
and the hardware state is broken. In this case it's ok to return an error,
and nothing will get displayed.
And yeah, you won't be able to do it every time, but if it's never for
some workload it's going to be a concern.
Anyway, we should at the very least document what we expect here.
Yes I should better document the drm panic feature, and the
get_scanout_buffer() interface.
Maxime
--
Jocelyn
