[v9,5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

[Jocelyn Falempe]

On 12/03/2024 17:44, Sui Jingfeng wrote:
> Hi,
> 
> I'm get attracted by your patch, so I want to comment.
> Please correct or ignore me if I say something wrong.
> 
> On 2024/3/7 17:14, Jocelyn Falempe wrote:
>> This was initialy done for imx6, but should work on most drivers
>> using drm_fb_dma_helper.
>>
>> v8:
>>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>>     (Thomas Zimmermann)
>>
>> v9:
>>   * go back to get_scanout_buffer()
>>   * move get_scanout_buffer() to plane helper functions
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++
>>   include/drm/drm_fb_dma_helper.h     |  4 +++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c 
>> b/drivers/gpu/drm/drm_fb_dma_helper.c
>> index 3b535ad1b07c..010327069ad4 100644
>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
>> @@ -15,6 +15,7 @@
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_dma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_plane.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/module.h>
>> @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct 
>> drm_device *drm,
>>       }
>>   }
>>   EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent);
>> +
>> +#if defined(CONFIG_DRM_PANIC)
>> +/**
>> + * @plane: DRM primary plane
>> + * @drm_scanout_buffer: scanout buffer for the panic handler
>> + * Returns: 0 or negative error code
>> + *
>> + * Generic get_scanout_buffer() implementation, for drivers that uses 
>> the
>> + * drm_fb_dma_helper.
>> + */
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb)
>> +{
>> +    struct drm_gem_dma_object *dma_obj;
>> +    struct drm_framebuffer *fb;
>> +
>> +    fb = plane->state->fb;
>> +    /* Only support linear modifier */
>> +    if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
>> +        return -ENODEV;
> 
> 
> I think, the framebuffer format check clause here should be moved to the 
> core layer.
> For example, move this check into thedrm_panic_is_format_supported() 
> function. And update the drm_panic_is_format_supported() function  to 
> make it take 'struct drm_framebuffer *fb'
> as argument. Because this check is needed for all implement, not driver 
> specific or
> implement specific.

I'm unsure about this. I think for some drivers it might be easier to 
give a memory buffer different from the plane's drm_framebuffer, and do 
their own specific things to display it. So forcing the use of a 
drm_framebuffer will remove some flexibility.

> 
> I know that some display controller support TILE format, but then you 
> can try to trigger modeset
> to let the display controller using linear format to display. Or, you 
> can also convert local
> linear buffer(with content pained) to the device specific TILED 
> framebuffer, then feed TILED
> framebuffer to the display controller to scanout. This is something like 
> reverse the process of
> resolve, but the convert program is running on the CPU.  As panic is not 
> performance critical,
> so it's possible. This maybe a bit more difficult, but the idea behind 
> this is that the goal of
> this drm_panic_gem_get_scanout_buffer() function is just to get a buffer 
> scanout-able. Other
> things beyond that (like format checking) can be moved to upper 
> level(the caller of it). So you
> don't need to modify(update) the specific implement if one day you have 
> some fancy idea implemented.
> 
> 

Correct me if I'm wrong, but the tiled format are mostly hardware 
dependent, and I don't think we can have a generic implementation, that 
can cover multiple hardware.

I want to add support for multi-planar format like YUV for drm_panic 
later, but for tiled buffer, I think it should be easier to deactivate 
tiling on the hardware itself.

>> +    dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> +
>> +    /* Buffer should be accessible from the CPU */
>> +    if (dma_obj->base.import_attach)
>> +        return -ENODEV;
>> +
>> +    /* Buffer should be already mapped to CPU */
>> +    if (!dma_obj->vaddr)
>> +        return -ENODEV;
> 
> 
> How about to call drm_gem_vmap_unlocked(dma_obj, &sb->map); ?

It is not safe to call it from panic context, because it takes locks:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem.c#L1212

It may lockup the panic handler, and prevent other panic routine to run 
(like kdump).
So it's better to call drm_gem_vmap_unlocked() when the driver prepares 
the buffer for the primary plane, than doing it from the panic handler.


Best regards,

-- 

Jocelyn


> 
> 
>> +    iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
>> +    sb->format = fb->format;
>> +    sb->height = fb->height;
>> +    sb->width = fb->width;
>> +    sb->pitch = fb->pitches[0];
>> +    return 0;
>> +}
>> +#else
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb)
>> +{
>> +    return -ENODEV;
>> +}
>> +#endif
>> +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer);
>> diff --git a/include/drm/drm_fb_dma_helper.h 
>> b/include/drm/drm_fb_dma_helper.h
>> index d5e036c57801..61f24c2aba2f 100644
>> --- a/include/drm/drm_fb_dma_helper.h
>> +++ b/include/drm/drm_fb_dma_helper.h
>> @@ -7,6 +7,7 @@
>>   struct drm_device;
>>   struct drm_framebuffer;
>>   struct drm_plane_state;
>> +struct drm_scanout_buffer;
>>   struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct 
>> drm_framebuffer *fb,
>>       unsigned int plane);
>> @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device 
>> *drm,
>>                     struct drm_plane_state *old_state,
>>                     struct drm_plane_state *state);
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb);
>> +
>>   #endif
>