[PATCH v7 2/9] drm/panic: Add a drm panic handler

[Thomas Zimmermann]

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.

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 at redhat.com>
> + */
> +
> +#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__ */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240117/c5f0c91a/attachment.sig>