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

[Jocelyn Falempe]

Hi,

On 09/04/2024 10:30, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.03.24 um 13:03 schrieb Jocelyn Falempe:
>> +/**
>> + * struct drm_scanout_buffer - DRM scanout buffer
>> + *
>> + * This structure holds the information necessary for drm_panic to 
>> draw the
>> + * panic screen, and display it.
>> + */
>> +struct drm_scanout_buffer {
>> +    /**
>> +     * @format:
>> +     *
>> +     * drm format of the scanout buffer.
>> +     */
>> +    const struct drm_format_info *format;
> 
> Newline here and among the other fields please.

Done in v12.
> 
>> +    /**
>> +     * @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;
> 
> I would make this an array of DRM_FORMAT_MAX_PLANES. Its functionality 
> is then equivalent to the fields in struct drm_framebuffer. Supporting 
> multiple color planes is the general expectation in the DRM code, even 
> if not all parts actually implement it. In the panic code, you simply 
> test that the scan-out format has only a single plane.
> 
>    struct iosys_map map[DRM_FORMAT_MAX_PLANES]
> 
Sure, that was on my todo list, done in v12.
> 
>> +    /**
>> +     * @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;
> 
> Also use an array of DRM_FORMAT_MAX_PLANES.
> 
>> +};
> 
> This data structure looks like something I could use for the 
> shadow-plane helpers. Expect it to be moved elsewhere at some point.

Yes, this can even be part of the struct drm_framebuffer.
> 
> Best regards
> Thomas
> 
> 

Thanks for the reviews,

-- 

Jocelyn