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

[Jocelyn Falempe <jfalempe@redhat.com>]

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

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