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

[Thomas Zimmermann]

Hi

Am 18.01.24 um 11:17 schrieb 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.

Sure, you could add this callback directly to the scanout buffer.

> 
> 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 ?

That makes me think about adding panic handling directly to the plane, 
something like this

   drm_plane_enable_panic_output(struct drm_plane*)

   drm_plane_set_panic_output_buffer(struct drm_plane*, struct 
drm_scanout_buffer*)

First time the driver calls drm_plane_enable_panic_output(), it sets up 
the panic handling for the given plane and maybe for DRM as a whole.

Each instance of the DRM panic handler operates on a single plane. Then 
during the pageflips, drm_plane_set_panic_output_buffer() updates the 
output buffer. The necessary sync/flush callbacks can be part of the 
drm_plane_funcs.

If/when the plane gets removed from the modesetting pipeline, the panic 
handling would be removed automatically.

Best regards
Thomas

> 
> Best regards,
> 

-- 
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/20240119/f20b80ec/attachment.sig>