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

[Thomas Zimmermann]

Hi

Am 09.04.24 um 16:11 schrieb 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.

Please not. These are two separate things (even though they look very 
similar).

Best regards
Thomas

>>
>> Best regards
>> Thomas
>>
>>
>
> Thanks for the reviews,
>

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