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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 20.02.24 um 11:16 schrieb Tony Lindgren:
> * Thomas Zimmermann <tzimmermann@suse.de> [240220 09:32]:
>> Am 20.02.24 um 09:56 schrieb Tony Lindgren:
>>> Oh right, yes omapdrm is operating on memory.
>> With the latest kernels, you should see a warning if helpers operate on the
>> wrong type of memory. [1][2]
> OK
>
>> [1] https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/video/fbdev/core/fb_io_fops.c#L16
>> [2] https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/video/fbdev/core/fb_sys_fops.c#L26
>>> The following test patch works for me.. Not sure about the tracking though.
>> I know that i915 doesn't track mmap'ed pages correctly and I've see systems
>> that do not update the framebuffer. IDK how/why this works with omapdrm.
> Sounds like the a similar issue might exist on omapdrm too then.
>
>>> Do you mean that tracking needs to be implemented if fb_deferred_io_mmap()
>>> did not work?
>> omap_fbdev_fb_mmap() appears to mmap the DMA memory pages directly to
>> userspace. So the fb_dirty callback won't be invoked when userspace writes
>> to this framebuffer memory.
>>
>> To implement tracking, you'd need to set fb_mmap to fb_deferred_io_mmap().
>> If you init omap_fb_ops with FB_DEFAULT_DEFERRED_OPS(), [3] you'd get that
>> automatically. fb_deferred_io_mmap() sets up the tracking whenever
>> userspaces writes to a mapped page.   You also need to init the write-back
>> mechanisms that calls fb_dirty for the tracked pages. You should be able to
>> duplicate the code from [4] into omapdrm.
> OK great. Updated test patch below with FB_DEFAULT_DEFERRED_OPS, maybe that's
> all there is to it if I follow you :) The fb_dirty part was already done in
> patch 1/2.

The changes below look good. You can test by instrumenting 
drm_fb_helper_deferred_io() with printk(). For testing, you can reduce 
the write-back frequency by setting helper->fbdefio.delay to a higher 
value. If you set it to HZ, it should only do a write-back once per 
second. Then do an mmap() from userspace and copy data into the memory 
region. It should print something from drm_fb_helper_deferred_io(). Best 
regards Thomas
>
>> [3] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L704
>> [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fbdev_generic.c#L119
> Regards,
>
> Tony
>
> 8< ---------------------------
> 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_DMAMEM_OPS(omap_fbdev,
> +				   drm_fb_helper_damage_range,
> +				   drm_fb_helper_damage_area)
> +
>   static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   		struct fb_info *fbi)
>   {
> @@ -76,15 +80,6 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   	return drm_fb_helper_pan_display(var, fbi);
>   }
>   
> -static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> -{
> -	struct drm_fb_helper *helper = info->par;
> -	struct drm_framebuffer *fb = helper->fb;
> -	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
> -
> -	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
> -}
> -
>   static void omap_fbdev_fb_destroy(struct fb_info *info)
>   {
>   	struct drm_fb_helper *helper = info->par;
> @@ -94,6 +89,7 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
>   
>   	DBG();
>   
> +	fb_deferred_io_cleanup(info);
>   	drm_fb_helper_fini(helper);
>   
>   	omap_gem_unpin(bo);
> @@ -106,15 +102,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(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_ioctl	= drm_fb_helper_ioctl,
> -	.fb_mmap	= omap_fbdev_fb_mmap,
>   	.fb_destroy	= omap_fbdev_fb_destroy,
>   };
>   
> @@ -213,6 +207,15 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   	fbi->fix.smem_start = dma_addr;
>   	fbi->fix.smem_len = bo->size;
>   
> +	/* deferred I/O */
> +	helper->fbdefio.delay = HZ / 20;
> +	helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +
> +	fbi->fbdefio = &helper->fbdefio;
> +	ret = fb_deferred_io_init(fbi);
> +	if (ret)
> +		goto fail;
> +
>   	/* if we have DMM, then we can use it for scrolling by just
>   	 * shuffling pages around in DMM rather than doing sw blit.
>   	 */
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -686,6 +686,10 @@ extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>   	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
>   	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
>   
> +#define FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(__prefix, __damage_range, __damage_area) \
> +	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
> +	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
> +
>   /*
>    * Initializes struct fb_ops for deferred I/O.
>    */

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