Re: [PATCH 2/2] drm/omapdrm: Fix console with deferred ops

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 19.02.24 um 15:19 schrieb Tony Lindgren:
> Commit 95da53d63dcf ("drm/omapdrm: Use regular fbdev I/O helpers")
> broke console because there is no damage handling in fb_sys_write()
> unlike we have in drm_fb_helper_sys_write().
>
> Let's fix the issue by using deferred ops with fb helpers for damage.
>
> Fixes: 95da53d63dcf ("drm/omapdrm: Use regular fbdev I/O helpers")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/gpu/drm/omapdrm/Kconfig      | 1 +
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 8 ++++++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
> --- a/drivers/gpu/drm/omapdrm/Kconfig
> +++ b/drivers/gpu/drm/omapdrm/Kconfig
> @@ -5,6 +5,7 @@ config DRM_OMAP
>   	depends on ARCH_OMAP2PLUS
>   	select DRM_KMS_HELPER
>   	select FB_DMAMEM_HELPERS if DRM_FBDEV_EMULATION
> +	select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION

Anything named _IOMEM_ is for framebuffer's in I/O memory space. Just 
keep DMAMEM_HELPERS with the few changes below.

>   	select VIDEOMODE_HELPERS
>   	select HDMI
>   	default n
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -51,6 +51,10 @@ static void pan_worker(struct work_struct *work)
>   	omap_gem_roll(bo, fbi->var.yoffset * npages);
>   }
>   
> +FB_GEN_DEFAULT_DEFERRED_IOMEM_OPS(omap_fbdev,
> +				  drm_fb_helper_damage_range,
> +				  drm_fb_helper_damage_area)
> +

Please create FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS() by duplicating 
FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS() in <linux/fb.h>

>   static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   		struct fb_info *fbi)
>   {
> @@ -106,13 +110,13 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
>   
>   static const struct fb_ops omap_fb_ops = {
>   	.owner = THIS_MODULE,
> -	__FB_DEFAULT_DMAMEM_OPS_RDWR,
> +	__FB_DEFAULT_DEFERRED_OPS_RDWR(omap_fbdev),
>   	.fb_check_var	= drm_fb_helper_check_var,
>   	.fb_set_par	= drm_fb_helper_set_par,
>   	.fb_setcmap	= drm_fb_helper_setcmap,
>   	.fb_blank	= drm_fb_helper_blank,
>   	.fb_pan_display = omap_fbdev_pan_display,
> -	__FB_DEFAULT_DMAMEM_OPS_DRAW,
> +	__FB_DEFAULT_DEFERRED_OPS_DRAW(omap_fbdev),
>   	.fb_ioctl	= drm_fb_helper_ioctl,
>   	.fb_mmap	= omap_fbdev_fb_mmap,

The write and draw callbacks track the written pages and flush them to 
the backbuffer. But mmap is a problem here, because mmap needs to do 
this as well. You'd have to use fb_deferred_io_mmap() here and call 
fb_deferred_io_init() in omap's fbdev init. See the generic fbdev in 
drm_fbdev_generic() for a working example. But IDK whether that works 
easily for omap's DMA memory. You have to mmap and track memory pages 
(i.e., struct page).

The easy solution is to clear the fb_mmap callback and mmap() will thne 
not be available to userspace.

Best regards
Thomas

>   	.fb_destroy	= omap_fbdev_fb_destroy,

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