[PATCH v11 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

[Jocelyn Falempe]

On 09/04/2024 10:21, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.03.24 um 13:03 schrieb Jocelyn Falempe:
>> 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)
> 
> I would not bother with CONFIG_DRM_PANIC for now and make the helper 
> generally available.

yes, that's a small function, let's keep it simple.
Done in v12
> 
>> +/**
>> + * @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)
> 
> It's neither really a function for panic handling nor a GEM function. 
> This helper needs to be called drm_fb_dma_get_scanout_buffer() IMHO.
> 
I agree, it matches the other functions prefixes in this file.
Done in v12

>> +{
>> +    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;
>> +
>> +    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;
>> +
>> +    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
> 

Thanks for the reviews,

-- 

Jocelyn