[PATCH v4 2/4] drm/panic: Add a drm panic handler

[Noralf Trønnes]

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.

Noralf.

[1]
https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
[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@amd.com/

> 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.
> 
>> 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.
> 
> Maxime