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

[Sui Jingfeng <sui.jingfeng@linux.dev>]

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


> +	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); ?

    

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