[PATCH 1/2] drm/omapdrm: Fix console by implementing fb_dirty

[PATCH 1/2] drm/omapdrm: Fix console by implementing fb_dirty

[Tony Lindgren]

The framebuffer console stopped updating with commit f231af498c29
("drm/fb-helper: Disconnect damage worker from update logic").

Let's fix the issue by implementing fb_dirty similar to what was done
with commit 039a72ce7e57 ("drm/i915/fbdev: Implement fb_dirty for intel
custom fb helper").

Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from update logic")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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
@@ -238,8 +238,20 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	return ret;
 }
 
+static int omap_fbdev_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip)
+{
+	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
+		return 0;
+
+	if (helper->fb->funcs->dirty)
+		return helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
+
+	return 0;
+}
+
 static const struct drm_fb_helper_funcs omap_fb_helper_funcs = {
 	.fb_probe = omap_fbdev_create,
+	.fb_dirty = omap_fbdev_dirty,
 };
 
 static struct drm_fb_helper *get_fb(struct fb_info *fbi)
-- 
2.43.1

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

[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
 	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)
+
 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,
 	.fb_destroy	= omap_fbdev_fb_destroy,
-- 
2.43.1

Re: [PATCH 1/2] drm/omapdrm: Fix console by implementing fb_dirty

[Thomas Zimmermann]

Hi

Am 19.02.24 um 15:19 schrieb Tony Lindgren:
> The framebuffer console stopped updating with commit f231af498c29
> ("drm/fb-helper: Disconnect damage worker from update logic").
>
> Let's fix the issue by implementing fb_dirty similar to what was done
> with commit 039a72ce7e57 ("drm/i915/fbdev: Implement fb_dirty for intel
> custom fb helper").
>
> Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from update logic")
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Looks reasonable.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> 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
> @@ -238,8 +238,20 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   	return ret;
>   }
>   
> +static int omap_fbdev_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip)
> +{
> +	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> +		return 0;
> +
> +	if (helper->fb->funcs->dirty)
> +		return helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
> +
> +	return 0;
> +}
> +
>   static const struct drm_fb_helper_funcs omap_fb_helper_funcs = {
>   	.fb_probe = omap_fbdev_create,
> +	.fb_dirty = omap_fbdev_dirty,
>   };
>   
>   static struct drm_fb_helper *get_fb(struct fb_info *fbi)

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

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

[Thomas Zimmermann]

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)

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

[Tony Lindgren]

* Thomas Zimmermann <tzimmermann@suse.de> [240219 16:43]:
> Am 19.02.24 um 15:19 schrieb Tony Lindgren:
> > --- 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.

Oh right, yes omapdrm is operating on memory.

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

OK

> >   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 following test patch works for me.. Not sure about the tracking though.
Do you mean that tracking needs to be implemented if fb_deferred_io_mmap()
did not work?

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

Sounds like that would break things for userspace.

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)
 {
@@ -80,9 +84,13 @@ 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);
+	struct drm_gem_object *bo;
 
-	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
+	bo = drm_gem_fb_get_obj(fb, 0);
+	if (!bo)
+		return -EINVAL;
+
+	return fb_deferred_io_mmap(info, vma);
 }
 
 static void omap_fbdev_fb_destroy(struct fb_info *info)
@@ -94,6 +102,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,13 +115,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,
 	.fb_destroy	= omap_fbdev_fb_destroy,
@@ -213,6 +222,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.
  */
-- 
2.43.1

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

[Thomas Zimmermann]

Hi

Am 20.02.24 um 09:56 schrieb Tony Lindgren:
> * Thomas Zimmermann <tzimmermann@suse.de> [240219 16:43]:
>> Am 19.02.24 um 15:19 schrieb Tony Lindgren:
>>> --- 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.
> 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]

[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

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

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

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

>
>> The easy solution is to clear the fb_mmap callback and mmap() will thne not
>> be available to userspace.
> Sounds like that would break things for userspace.

Yes, probably.

>
> 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)
>   {
> @@ -80,9 +84,13 @@ 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);
> +	struct drm_gem_object *bo;
>   
> -	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
> +	bo = drm_gem_fb_get_obj(fb, 0);
> +	if (!bo)
> +		return -EINVAL;
> +
> +	return fb_deferred_io_mmap(info, vma);
>   }
>   
>   static void omap_fbdev_fb_destroy(struct fb_info *info)
> @@ -94,6 +102,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,13 +115,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,
>   	.fb_destroy	= omap_fbdev_fb_destroy,
> @@ -213,6 +222,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)

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

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

> [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.
  */
-- 
2.43.1

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

[Thomas Zimmermann]

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)

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

[Tony Lindgren]

* Thomas Zimmermann <tzimmermann@suse.de> [240220 10:42]:
> 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

OK thanks I tested with fbtest.c I found [0] with some trace_printk
added and it works nicely :) Will post v2 set of patches.

Regards,

Tony

[0] https://gist.github.com/rafalrusin/1482697