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

[Jocelyn Falempe]

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 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__ */
>