[PATCH v2 0/3] fbdev: Decouple deferred I/O from struct page

[PATCH v2 0/3] fbdev: Decouple deferred I/O from struct page

[Thomas Zimmermann]

Rework the fbdev deferred-I/O to not interfere with fields of struct
page. All references from deferred-I/O code to fields in struct page
are gone. The rsp state is help in a separate pageref structure.

Version 1 of this patchset was part of a larger attempt to improve
GEM SHMEM support. [1] The patches can already be merged to resolve
a long-standing issue in the fbdev code.

[1] https://lore.kernel.org/dri-devel/20220303205839.28484-1-tzimmermann@suse.de/

Thomas Zimmermann (3):
  fbdev: Put mmap for deferred I/O into drivers
  fbdev: Track deferred-I/O pages in pageref struct
  fbdev: Refactor implementation of page_mkwrite

 drivers/gpu/drm/drm_fb_helper.c        |  10 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |   6 +-
 drivers/hid/hid-picolcd_fb.c           |   1 +
 drivers/staging/fbtft/fbtft-core.c     |   6 +-
 drivers/video/fbdev/broadsheetfb.c     |   6 +-
 drivers/video/fbdev/core/fb_defio.c    | 213 +++++++++++++++++--------
 drivers/video/fbdev/core/fbmem.c       |  19 ++-
 drivers/video/fbdev/hecubafb.c         |   1 +
 drivers/video/fbdev/hyperv_fb.c        |   6 +-
 drivers/video/fbdev/metronomefb.c      |   6 +-
 drivers/video/fbdev/sh_mobile_lcdcfb.c |  12 +-
 drivers/video/fbdev/smscufx.c          |   8 +-
 drivers/video/fbdev/ssd1307fb.c        |   1 +
 drivers/video/fbdev/udlfb.c            |   8 +-
 drivers/video/fbdev/xen-fbfront.c      |   6 +-
 include/linux/fb.h                     |  11 +-
 16 files changed, 221 insertions(+), 99 deletions(-)


base-commit: 0e7deff6446a4ba2c75f499a0bfa80cd6a15c129
-- 
2.36.0

[PATCH v2 0/3] fbdev: Decouple deferred I/O from struct page

[Thomas Zimmermann]

Rework the fbdev deferred-I/O to not interfere with fields of struct
page. All references from deferred-I/O code to fields in struct page
are gone. The rsp state is help in a separate pageref structure.

Version 1 of this patchset was part of a larger attempt to improve
GEM SHMEM support. [1] The patches can already be merged to resolve
a long-standing issue in the fbdev code.

[1] https://lore.kernel.org/dri-devel/20220303205839.28484-1-tzimmermann@suse.de/

Thomas Zimmermann (3):
  fbdev: Put mmap for deferred I/O into drivers
  fbdev: Track deferred-I/O pages in pageref struct
  fbdev: Refactor implementation of page_mkwrite

 drivers/gpu/drm/drm_fb_helper.c        |  10 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |   6 +-
 drivers/hid/hid-picolcd_fb.c           |   1 +
 drivers/staging/fbtft/fbtft-core.c     |   6 +-
 drivers/video/fbdev/broadsheetfb.c     |   6 +-
 drivers/video/fbdev/core/fb_defio.c    | 213 +++++++++++++++++--------
 drivers/video/fbdev/core/fbmem.c       |  19 ++-
 drivers/video/fbdev/hecubafb.c         |   1 +
 drivers/video/fbdev/hyperv_fb.c        |   6 +-
 drivers/video/fbdev/metronomefb.c      |   6 +-
 drivers/video/fbdev/sh_mobile_lcdcfb.c |  12 +-
 drivers/video/fbdev/smscufx.c          |   8 +-
 drivers/video/fbdev/ssd1307fb.c        |   1 +
 drivers/video/fbdev/udlfb.c            |   8 +-
 drivers/video/fbdev/xen-fbfront.c      |   6 +-
 include/linux/fb.h                     |  11 +-
 16 files changed, 221 insertions(+), 99 deletions(-)


base-commit: 0e7deff6446a4ba2c75f499a0bfa80cd6a15c129
-- 
2.36.0

[PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Thomas Zimmermann]

The fbdev mmap function fb_mmap() unconditionally overrides the
driver's implementation if deferred I/O has been activated. This
makes it hard to implement mmap with anything but a vmalloc()'ed
software buffer. That is specifically a problem for DRM, where
video memory is maintained by a memory manager.

Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.

v2:
	* print a helpful error message if the defio setup is
	  incorrect (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c        |  4 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |  1 +
 drivers/hid/hid-picolcd_fb.c           |  1 +
 drivers/staging/fbtft/fbtft-core.c     |  1 +
 drivers/video/fbdev/broadsheetfb.c     |  1 +
 drivers/video/fbdev/core/fb_defio.c    |  1 +
 drivers/video/fbdev/core/fbmem.c       | 19 +++++++++----------
 drivers/video/fbdev/hecubafb.c         |  1 +
 drivers/video/fbdev/hyperv_fb.c        |  1 +
 drivers/video/fbdev/metronomefb.c      |  1 +
 drivers/video/fbdev/sh_mobile_lcdcfb.c |  6 ++++++
 drivers/video/fbdev/smscufx.c          |  3 +++
 drivers/video/fbdev/ssd1307fb.c        |  1 +
 drivers/video/fbdev/udlfb.c            |  3 +++
 drivers/video/fbdev/xen-fbfront.c      |  1 +
 15 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..d06ce0e92d66 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2118,7 +2118,9 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 
-	if (fb_helper->dev->driver->gem_prime_mmap)
+	if (drm_fbdev_use_shadow_fb(fb_helper))
+		return fb_deferred_io_mmap(info, vma);
+	else if (fb_helper->dev->driver->gem_prime_mmap)
 		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
 	else
 		return -ENODEV;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index adf17c740656..02a4a4fa3da3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -619,6 +619,7 @@ static const struct fb_ops vmw_fb_ops = {
 	.fb_imageblit = vmw_fb_imageblit,
 	.fb_pan_display = vmw_fb_pan_display,
 	.fb_blank = vmw_fb_blank,
+	.fb_mmap = fb_deferred_io_mmap,
 };
 
 int vmw_fb_init(struct vmw_private *vmw_priv)
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 33c102a60992..78515e6bacf0 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -428,6 +428,7 @@ static const struct fb_ops picolcdfb_ops = {
 	.fb_imageblit = picolcd_fb_imageblit,
 	.fb_check_var = picolcd_fb_check_var,
 	.fb_set_par   = picolcd_set_par,
+	.fb_mmap      = fb_deferred_io_mmap,
 };
 
 
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9c4d797e7ae4..d4e14f7c9d57 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -652,6 +652,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	fbops->fb_imageblit =      fbtft_fb_imageblit;
 	fbops->fb_setcolreg =      fbtft_fb_setcolreg;
 	fbops->fb_blank     =      fbtft_fb_blank;
+	fbops->fb_mmap      =      fb_deferred_io_mmap;
 
 	fbdefio->delay =           HZ / fps;
 	fbdefio->sort_pagelist =   true;
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index b9054f658838..528bc0902338 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -1055,6 +1055,7 @@ static const struct fb_ops broadsheetfb_ops = {
 	.fb_fillrect	= broadsheetfb_fillrect,
 	.fb_copyarea	= broadsheetfb_copyarea,
 	.fb_imageblit	= broadsheetfb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io broadsheetfb_defio = {
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 6aaf6d0abf39..6924d489a289 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	vma->vm_private_data = info;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
 
 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 84427470367b..52440e3f8f69 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1334,7 +1334,6 @@ static int
 fb_mmap(struct file *file, struct vm_area_struct * vma)
 {
 	struct fb_info *info = file_fb_info(file);
-	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
 	unsigned long mmio_pgoff;
 	unsigned long start;
 	u32 len;
@@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
 		return -ENODEV;
 	mutex_lock(&info->mm_lock);
 
-	fb_mmap_fn = info->fbops->fb_mmap;
-
-#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
-	if (info->fbdefio)
-		fb_mmap_fn = fb_deferred_io_mmap;
-#endif
-
-	if (fb_mmap_fn) {
+	if (info->fbops->fb_mmap) {
 		int res;
 
 		/*
@@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
 		 * SME protection is removed ahead of the call
 		 */
 		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-		res = fb_mmap_fn(info, vma);
+		res = info->fbops->fb_mmap(info, vma);
 		mutex_unlock(&info->mm_lock);
 		return res;
 	}
 
+	/*
+	 * FB deferred I/O wants you to handle mmap in your drivers. At a
+	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
+	 */
+	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
+		return -ENODEV;
+
 	/*
 	 * Ugh. This can be either the frame buffer mapping, or
 	 * if pgoff points past it, the mmio mapping.
diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index 00d77105161a..2c483e2cf9ec 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -204,6 +204,7 @@ static const struct fb_ops hecubafb_ops = {
 	.fb_fillrect	= hecubafb_fillrect,
 	.fb_copyarea	= hecubafb_copyarea,
 	.fb_imageblit	= hecubafb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io hecubafb_defio = {
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index c8e0ea27caf1..d7f6abf827b9 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -909,6 +909,7 @@ static const struct fb_ops hvfb_ops = {
 	.fb_copyarea = hvfb_cfb_copyarea,
 	.fb_imageblit = hvfb_cfb_imageblit,
 	.fb_blank = hvfb_blank,
+	.fb_mmap = fb_deferred_io_mmap,
 };
 
 
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index af858dd23ea6..2541f2fe065b 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -564,6 +564,7 @@ static const struct fb_ops metronomefb_ops = {
 	.fb_fillrect	= metronomefb_fillrect,
 	.fb_copyarea	= metronomefb_copyarea,
 	.fb_imageblit	= metronomefb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io metronomefb_defio = {
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index aa4ebe3192ec..1dc5079e4518 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -1483,6 +1483,9 @@ sh_mobile_lcdc_overlay_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct sh_mobile_lcdc_overlay *ovl = info->par;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	return dma_mmap_coherent(ovl->channel->lcdc->dev, vma, ovl->fb_mem,
 				 ovl->dma_handle, ovl->fb_size);
 }
@@ -1957,6 +1960,9 @@ sh_mobile_lcdc_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct sh_mobile_lcdc_chan *ch = info->par;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	return dma_mmap_coherent(ch->lcdc->dev, vma, ch->fb_mem,
 				 ch->dma_handle, ch->fb_size);
 }
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 28768c272b73..9383f8d0914c 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -779,6 +779,9 @@ static int ufx_ops_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long page, pos;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;
 	if (size > info->fix.smem_len)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index c6d5df31111d..7547b4628afc 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -368,6 +368,7 @@ static const struct fb_ops ssd1307fb_ops = {
 	.fb_fillrect	= ssd1307fb_fillrect,
 	.fb_copyarea	= ssd1307fb_copyarea,
 	.fb_imageblit	= ssd1307fb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static void ssd1307fb_deferred_io(struct fb_info *info,
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index b6ec0b8e2b72..9e5a33396ef9 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -326,6 +326,9 @@ static int dlfb_ops_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long page, pos;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;
 	if (size > info->fix.smem_len)
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 6826f986da43..6c8846eba2fb 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -338,6 +338,7 @@ static const struct fb_ops xenfb_fb_ops = {
 	.fb_imageblit	= xenfb_imageblit,
 	.fb_check_var	= xenfb_check_var,
 	.fb_set_par     = xenfb_set_par,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static irqreturn_t xenfb_event_handler(int rq, void *dev_id)
-- 
2.36.0

[PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Thomas Zimmermann]

The fbdev mmap function fb_mmap() unconditionally overrides the
driver's implementation if deferred I/O has been activated. This
makes it hard to implement mmap with anything but a vmalloc()'ed
software buffer. That is specifically a problem for DRM, where
video memory is maintained by a memory manager.

Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.

v2:
	* print a helpful error message if the defio setup is
	  incorrect (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c        |  4 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |  1 +
 drivers/hid/hid-picolcd_fb.c           |  1 +
 drivers/staging/fbtft/fbtft-core.c     |  1 +
 drivers/video/fbdev/broadsheetfb.c     |  1 +
 drivers/video/fbdev/core/fb_defio.c    |  1 +
 drivers/video/fbdev/core/fbmem.c       | 19 +++++++++----------
 drivers/video/fbdev/hecubafb.c         |  1 +
 drivers/video/fbdev/hyperv_fb.c        |  1 +
 drivers/video/fbdev/metronomefb.c      |  1 +
 drivers/video/fbdev/sh_mobile_lcdcfb.c |  6 ++++++
 drivers/video/fbdev/smscufx.c          |  3 +++
 drivers/video/fbdev/ssd1307fb.c        |  1 +
 drivers/video/fbdev/udlfb.c            |  3 +++
 drivers/video/fbdev/xen-fbfront.c      |  1 +
 15 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..d06ce0e92d66 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2118,7 +2118,9 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 
-	if (fb_helper->dev->driver->gem_prime_mmap)
+	if (drm_fbdev_use_shadow_fb(fb_helper))
+		return fb_deferred_io_mmap(info, vma);
+	else if (fb_helper->dev->driver->gem_prime_mmap)
 		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
 	else
 		return -ENODEV;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index adf17c740656..02a4a4fa3da3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -619,6 +619,7 @@ static const struct fb_ops vmw_fb_ops = {
 	.fb_imageblit = vmw_fb_imageblit,
 	.fb_pan_display = vmw_fb_pan_display,
 	.fb_blank = vmw_fb_blank,
+	.fb_mmap = fb_deferred_io_mmap,
 };
 
 int vmw_fb_init(struct vmw_private *vmw_priv)
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 33c102a60992..78515e6bacf0 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -428,6 +428,7 @@ static const struct fb_ops picolcdfb_ops = {
 	.fb_imageblit = picolcd_fb_imageblit,
 	.fb_check_var = picolcd_fb_check_var,
 	.fb_set_par   = picolcd_set_par,
+	.fb_mmap      = fb_deferred_io_mmap,
 };
 
 
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9c4d797e7ae4..d4e14f7c9d57 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -652,6 +652,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	fbops->fb_imageblit =      fbtft_fb_imageblit;
 	fbops->fb_setcolreg =      fbtft_fb_setcolreg;
 	fbops->fb_blank     =      fbtft_fb_blank;
+	fbops->fb_mmap      =      fb_deferred_io_mmap;
 
 	fbdefio->delay =           HZ / fps;
 	fbdefio->sort_pagelist =   true;
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index b9054f658838..528bc0902338 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -1055,6 +1055,7 @@ static const struct fb_ops broadsheetfb_ops = {
 	.fb_fillrect	= broadsheetfb_fillrect,
 	.fb_copyarea	= broadsheetfb_copyarea,
 	.fb_imageblit	= broadsheetfb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io broadsheetfb_defio = {
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 6aaf6d0abf39..6924d489a289 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	vma->vm_private_data = info;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
 
 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 84427470367b..52440e3f8f69 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1334,7 +1334,6 @@ static int
 fb_mmap(struct file *file, struct vm_area_struct * vma)
 {
 	struct fb_info *info = file_fb_info(file);
-	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
 	unsigned long mmio_pgoff;
 	unsigned long start;
 	u32 len;
@@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
 		return -ENODEV;
 	mutex_lock(&info->mm_lock);
 
-	fb_mmap_fn = info->fbops->fb_mmap;
-
-#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
-	if (info->fbdefio)
-		fb_mmap_fn = fb_deferred_io_mmap;
-#endif
-
-	if (fb_mmap_fn) {
+	if (info->fbops->fb_mmap) {
 		int res;
 
 		/*
@@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
 		 * SME protection is removed ahead of the call
 		 */
 		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-		res = fb_mmap_fn(info, vma);
+		res = info->fbops->fb_mmap(info, vma);
 		mutex_unlock(&info->mm_lock);
 		return res;
 	}
 
+	/*
+	 * FB deferred I/O wants you to handle mmap in your drivers. At a
+	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
+	 */
+	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
+		return -ENODEV;
+
 	/*
 	 * Ugh. This can be either the frame buffer mapping, or
 	 * if pgoff points past it, the mmio mapping.
diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index 00d77105161a..2c483e2cf9ec 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -204,6 +204,7 @@ static const struct fb_ops hecubafb_ops = {
 	.fb_fillrect	= hecubafb_fillrect,
 	.fb_copyarea	= hecubafb_copyarea,
 	.fb_imageblit	= hecubafb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io hecubafb_defio = {
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index c8e0ea27caf1..d7f6abf827b9 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -909,6 +909,7 @@ static const struct fb_ops hvfb_ops = {
 	.fb_copyarea = hvfb_cfb_copyarea,
 	.fb_imageblit = hvfb_cfb_imageblit,
 	.fb_blank = hvfb_blank,
+	.fb_mmap = fb_deferred_io_mmap,
 };
 
 
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index af858dd23ea6..2541f2fe065b 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -564,6 +564,7 @@ static const struct fb_ops metronomefb_ops = {
 	.fb_fillrect	= metronomefb_fillrect,
 	.fb_copyarea	= metronomefb_copyarea,
 	.fb_imageblit	= metronomefb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static struct fb_deferred_io metronomefb_defio = {
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index aa4ebe3192ec..1dc5079e4518 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -1483,6 +1483,9 @@ sh_mobile_lcdc_overlay_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct sh_mobile_lcdc_overlay *ovl = info->par;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	return dma_mmap_coherent(ovl->channel->lcdc->dev, vma, ovl->fb_mem,
 				 ovl->dma_handle, ovl->fb_size);
 }
@@ -1957,6 +1960,9 @@ sh_mobile_lcdc_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct sh_mobile_lcdc_chan *ch = info->par;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	return dma_mmap_coherent(ch->lcdc->dev, vma, ch->fb_mem,
 				 ch->dma_handle, ch->fb_size);
 }
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 28768c272b73..9383f8d0914c 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -779,6 +779,9 @@ static int ufx_ops_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long page, pos;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;
 	if (size > info->fix.smem_len)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index c6d5df31111d..7547b4628afc 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -368,6 +368,7 @@ static const struct fb_ops ssd1307fb_ops = {
 	.fb_fillrect	= ssd1307fb_fillrect,
 	.fb_copyarea	= ssd1307fb_copyarea,
 	.fb_imageblit	= ssd1307fb_imageblit,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static void ssd1307fb_deferred_io(struct fb_info *info,
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index b6ec0b8e2b72..9e5a33396ef9 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -326,6 +326,9 @@ static int dlfb_ops_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long page, pos;
 
+	if (info->fbdefio)
+		return fb_deferred_io_mmap(info, vma);
+
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;
 	if (size > info->fix.smem_len)
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 6826f986da43..6c8846eba2fb 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -338,6 +338,7 @@ static const struct fb_ops xenfb_fb_ops = {
 	.fb_imageblit	= xenfb_imageblit,
 	.fb_check_var	= xenfb_check_var,
 	.fb_set_par     = xenfb_set_par,
+	.fb_mmap	= fb_deferred_io_mmap,
 };
 
 static irqreturn_t xenfb_event_handler(int rq, void *dev_id)
-- 
2.36.0

[PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Thomas Zimmermann]

Store the per-page state for fbdev's deferred I/O in struct
fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
that have to be written back to video memory. Update all affected
drivers.

As with pages before, fbdev acquires a pageref when an mmaped page
of the framebuffer is being written to. It holds the pageref in a
list of all currently written pagerefs until it flushes the written
pages to video memory. Writeback occurs periodically. After writeback
fbdev releases all pagerefs and builds up a new dirty list until the
next writeback occurs.

Using pagerefs has a number of benefits.

For pages of the framebuffer, the deferred I/O code used struct
page.lru as an entry into the list of dirty pages. The lru field is
owned by the page cache, which makes deferred I/O incompatible with
some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
struct fb_deferred_io_pageref now provides an entry into a list of
dirty framebuffer pages, freeing lru for use with the page cache.

Drivers also assumed that struct page.index is the page offset into
the framebuffer. This is not true for DRM buffers, which are located
at various offset within a mapped area. struct fb_deferred_io_pageref
explicitly stores an offset into the framebuffer. struct page.index
is now only the page offset into the mapped area.

These changes will allow DRM to use fbdev deferred I/O without an
intermediate shadow buffer.

v2:
	* minor fixes in commit message

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c        |   6 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |   5 +-
 drivers/staging/fbtft/fbtft-core.c     |   5 +-
 drivers/video/fbdev/broadsheetfb.c     |   5 +-
 drivers/video/fbdev/core/fb_defio.c    | 156 ++++++++++++++++---------
 drivers/video/fbdev/hyperv_fb.c        |   5 +-
 drivers/video/fbdev/metronomefb.c      |   5 +-
 drivers/video/fbdev/sh_mobile_lcdcfb.c |   6 +-
 drivers/video/fbdev/smscufx.c          |   5 +-
 drivers/video/fbdev/udlfb.c            |   5 +-
 drivers/video/fbdev/xen-fbfront.c      |   5 +-
 include/linux/fb.h                     |  11 +-
 12 files changed, 145 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d06ce0e92d66..dd1d72d58b35 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -717,13 +717,13 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
 			       struct list_head *pagelist)
 {
 	unsigned long start, end, min, max;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	struct drm_rect damage_area;
 
 	min = ULONG_MAX;
 	max = 0;
-	list_for_each_entry(page, pagelist, lru) {
-		start = page->index << PAGE_SHIFT;
+	list_for_each_entry(pageref, pagelist, list) {
+		start = pageref->offset;
 		end = start + PAGE_SIZE;
 		min = min(min, start);
 		max = max(max, end);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index 02a4a4fa3da3..db02e17e12b6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
 	struct vmw_fb_par *par = info->par;
 	unsigned long start, end, min, max;
 	unsigned long flags;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	int y1, y2;
 
 	min = ULONG_MAX;
 	max = 0;
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		start = page->index << PAGE_SHIFT;
 		end = start + PAGE_SIZE - 1;
 		min = min(min, start);
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index d4e14f7c9d57..e9662822e1ef 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -326,7 +326,7 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagelist)
 {
 	struct fbtft_par *par = info->par;
 	unsigned int dirty_lines_start, dirty_lines_end;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long index;
 	unsigned int y_low = 0, y_high = 0;
 	int count = 0;
@@ -340,7 +340,8 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagelist)
 	spin_unlock(&par->dirty_lock);
 
 	/* Mark display lines as dirty */
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		count++;
 		index = page->index << PAGE_SHIFT;
 		y_low = index / info->fix.line_length;
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index 528bc0902338..6afc6ef4cb5e 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -934,7 +934,7 @@ static void broadsheetfb_dpy_deferred_io(struct fb_info *info,
 {
 	u16 y1 = 0, h = 0;
 	int prev_index = -1;
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	int h_inc;
 	u16 yres = info->var.yres;
@@ -944,7 +944,8 @@ static void broadsheetfb_dpy_deferred_io(struct fb_info *info,
 	h_inc = DIV_ROUND_UP(PAGE_SIZE , xres);
 
 	/* walk the written page list and swizzle the data */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		if (prev_index < 0) {
 			/* just starting so assign first page */
 			y1 = (cur->index << PAGE_SHIFT) / xres;
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 6924d489a289..a03b9c64fc61 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
 	return page;
 }
 
+static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
+								 unsigned long offset,
+								 struct page *page)
+{
+	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct list_head *pos = &fbdefio->pagelist;
+	unsigned long pgoff = offset >> PAGE_SHIFT;
+	struct fb_deferred_io_pageref *pageref, *cur;
+
+	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
+		return NULL; /* incorrect allocation size */
+
+	/* 1:1 mapping between pageref and page offset */
+	pageref = &info->pagerefs[pgoff];
+
+	/*
+	 * This check is to catch the case where a new process could start
+	 * writing to the same page through a new PTE. This new access
+	 * can cause a call to .page_mkwrite even if the original process'
+	 * PTE is marked writable.
+	 */
+	if (!list_empty(&pageref->list))
+		goto pageref_already_added;
+
+	pageref->page = page;
+	pageref->offset = pgoff << PAGE_SHIFT;
+
+	if (unlikely(fbdefio->sort_pagelist)) {
+		/*
+		 * We loop through the list of pagerefs before adding, in
+		 * order to keep the pagerefs sorted. This has significant
+		 * overhead of O(n^2) with n being the number of written
+		 * pages. If possible, drivers should try to work with
+		 * unsorted page lists instead.
+		 */
+		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
+			if (cur > pageref)
+				break;
+		}
+		pos = &cur->list;
+	}
+
+	list_add_tail(&pageref->list, pos);
+
+pageref_already_added:
+	return pageref;
+}
+
+static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
+				       struct fb_info *info)
+{
+	list_del_init(&pageref->list);
+}
+
 /* this is to find and return the vmalloc-ed fb pages */
 static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
 {
@@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
 		printk(KERN_ERR "no mapping available\n");
 
 	BUG_ON(!page->mapping);
-	page->index = vmf->pgoff;
+	page->index = vmf->pgoff; /* for page_mkclean() */
 
 	vmf->page = page;
 	return 0;
@@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	struct page *page = vmf->page;
 	struct fb_info *info = vmf->vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct list_head *pos = &fbdefio->pagelist;
+	struct fb_deferred_io_pageref *pageref;
+	unsigned long offset;
+	vm_fault_t ret;
+
+	offset = (vmf->address - vmf->vma->vm_start);
 
 	/* this is a callback we get when userspace first tries to
 	write to the page. we schedule a workqueue. that workqueue
@@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
 		fbdefio->first_io(info);
 
+	pageref = fb_deferred_io_pageref_get(info, offset, page);
+	if (WARN_ON_ONCE(!pageref)) {
+		ret = VM_FAULT_OOM;
+		goto err_mutex_unlock;
+	}
+
 	/*
 	 * We want the page to remain locked from ->page_mkwrite until
 	 * the PTE is marked dirty to avoid page_mkclean() being called
@@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	 * Do this by locking the page here and informing the caller
 	 * about it with VM_FAULT_LOCKED.
 	 */
-	lock_page(page);
-
-	/*
-	 * This check is to catch the case where a new process could start
-	 * writing to the same page through a new PTE. This new access
-	 * can cause a call to .page_mkwrite even if the original process'
-	 * PTE is marked writable.
-	 *
-	 * TODO: The lru field is owned by the page cache; hence the name.
-	 *       We dequeue in fb_deferred_io_work() after flushing the
-	 *       page's content into video memory. Instead of lru, fbdefio
-	 *       should have it's own field.
-	 */
-	if (!list_empty(&page->lru))
-		goto page_already_added;
-
-	if (unlikely(fbdefio->sort_pagelist)) {
-		/*
-		 * We loop through the pagelist before adding in order to
-		 * keep the pagelist sorted. This has significant overhead
-		 * of O(n^2) with n being the number of written pages. If
-		 * possible, drivers should try to work with unsorted page
-		 * lists instead.
-		 */
-		struct page *cur;
-
-		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
-			if (cur->index > page->index)
-				break;
-		}
-		pos = &cur->lru;
-	}
-
-	list_add_tail(&page->lru, pos);
+	lock_page(pageref->page);
 
-page_already_added:
 	mutex_unlock(&fbdefio->lock);
 
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
 	return VM_FAULT_LOCKED;
+
+err_mutex_unlock:
+	mutex_unlock(&fbdefio->lock);
+	return ret;
 }
 
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
@@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
 {
-	struct fb_info *info = container_of(work, struct fb_info,
-						deferred_work.work);
-	struct list_head *node, *next;
-	struct page *cur;
+	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
+	struct fb_deferred_io_pageref *pageref, *next;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 
 	/* here we mkclean the pages, then do all deferred IO */
 	mutex_lock(&fbdefio->lock);
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		lock_page(cur);
 		page_mkclean(cur);
 		unlock_page(cur);
@@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
 	fbdefio->deferred_io(info, &fbdefio->pagelist);
 
 	/* clear the list */
-	list_for_each_safe(node, next, &fbdefio->pagelist) {
-		list_del_init(node);
-	}
+	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
+		fb_deferred_io_pageref_put(pageref, info);
+
 	mutex_unlock(&fbdefio->lock);
 }
 
-void fb_deferred_io_init(struct fb_info *info)
+int fb_deferred_io_init(struct fb_info *info)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct page *page;
-	unsigned int i;
+	struct fb_deferred_io_pageref *pagerefs;
+	unsigned long npagerefs, i;
+	int ret;
 
 	BUG_ON(!fbdefio);
+
+	if (WARN_ON(!info->fix.smem_len))
+		return -EINVAL;
+
 	mutex_init(&fbdefio->lock);
 	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
 	INIT_LIST_HEAD(&fbdefio->pagelist);
 	if (fbdefio->delay == 0) /* set a default of 1 s */
 		fbdefio->delay = HZ;
 
-	/* initialize all the page lists one time */
-	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
-		page = fb_deferred_io_page(info, i);
-		INIT_LIST_HEAD(&page->lru);
+	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
+
+	/* alloc a page ref for each page of the display memory */
+	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
+	if (!pagerefs) {
+		ret = -ENOMEM;
+		goto err;
 	}
+	for (i = 0; i < npagerefs; ++i)
+		INIT_LIST_HEAD(&pagerefs[i].list);
+	info->npagerefs = npagerefs;
+	info->pagerefs = pagerefs;
+
+	return 0;
+
+err:
+	mutex_destroy(&fbdefio->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_init);
 
@@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 		page->mapping = NULL;
 	}
 
+	kvfree(info->pagerefs);
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index d7f6abf827b9..51eb57ea68f7 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -424,7 +424,7 @@ static void synthvid_deferred_io(struct fb_info *p,
 				 struct list_head *pagelist)
 {
 	struct hvfb_par *par = p->par;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long start, end;
 	int y1, y2, miny, maxy;
 
@@ -437,7 +437,8 @@ static void synthvid_deferred_io(struct fb_info *p,
 	 * in synthvid_update function by clamping the y2
 	 * value to yres.
 	 */
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		start = page->index << PAGE_SHIFT;
 		end = start + PAGE_SIZE - 1;
 		y1 = start / p->fix.line_length;
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 2541f2fe065b..8352fa3f4cef 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -469,12 +469,13 @@ static void metronomefb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
 	u16 cksum;
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct metronomefb_par *par = info->par;
 
 	/* walk the written page list and swizzle the data */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		cksum = metronomefb_dpy_update_page(par,
 					(cur->index << PAGE_SHIFT));
 		par->metromem_img_csum -= par->csum_table[cur->index];
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index 1dc5079e4518..7fcc85352033 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -440,13 +440,15 @@ static int sh_mobile_lcdc_sginit(struct fb_info *info,
 {
 	struct sh_mobile_lcdc_chan *ch = info->par;
 	unsigned int nr_pages_max = ch->fb_size >> PAGE_SHIFT;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	int nr_pages = 0;
 
 	sg_init_table(ch->sglist, nr_pages_max);
 
-	list_for_each_entry(page, pagelist, lru)
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		sg_set_page(&ch->sglist[nr_pages++], page, PAGE_SIZE, 0);
+	}
 
 	return nr_pages;
 }
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 9383f8d0914c..eb108f56ea04 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -958,7 +958,7 @@ static void ufx_ops_fillrect(struct fb_info *info,
 static void ufx_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct ufx_data *dev = info->par;
 
@@ -969,9 +969,10 @@ static void ufx_dpy_deferred_io(struct fb_info *info,
 		return;
 
 	/* walk the written page list and render each to device */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
 		/* create a rectangle of full screen width that encloses the
 		 * entire dirty framebuffer page */
+		struct page *cur = pageref->page;
 		const int x = 0;
 		const int width = dev->info->var.xres;
 		const int y = (cur->index << PAGE_SHIFT) / (width * 2);
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 9e5a33396ef9..8249ed8700d4 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -784,7 +784,7 @@ static void dlfb_ops_fillrect(struct fb_info *info,
 static void dlfb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct dlfb_data *dlfb = info->par;
 	struct urb *urb;
@@ -811,7 +811,8 @@ static void dlfb_dpy_deferred_io(struct fb_info *info,
 	cmd = urb->transfer_buffer;
 
 	/* walk the written page list and render each to device */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 
 		if (dlfb_render_hline(dlfb, &urb, (char *) info->fix.smem_start,
 				  &cmd, cur->index << PAGE_SHIFT,
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 6c8846eba2fb..608fcde767d3 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -185,13 +185,14 @@ static void xenfb_deferred_io(struct fb_info *fb_info,
 			      struct list_head *pagelist)
 {
 	struct xenfb_info *info = fb_info->par;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long beg, end;
 	int y1, y2, miny, maxy;
 
 	miny = INT_MAX;
 	maxy = 0;
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		beg = page->index << PAGE_SHIFT;
 		end = beg + PAGE_SIZE - 1;
 		y1 = beg / fb_info->fix.line_length;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f95da1af9ff6..a332590c0fae 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -201,6 +201,13 @@ struct fb_pixmap {
 };
 
 #ifdef CONFIG_FB_DEFERRED_IO
+struct fb_deferred_io_pageref {
+	struct page *page;
+	unsigned long offset;
+	/* private */
+	struct list_head list;
+};
+
 struct fb_deferred_io {
 	/* delay between mkwrite and deferred handler */
 	unsigned long delay;
@@ -468,6 +475,8 @@ struct fb_info {
 #endif
 #ifdef CONFIG_FB_DEFERRED_IO
 	struct delayed_work deferred_work;
+	unsigned long npagerefs;
+	struct fb_deferred_io_pageref *pagerefs;
 	struct fb_deferred_io *fbdefio;
 #endif
 
@@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
 
 /* drivers/video/fb_defio.c */
 int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
-extern void fb_deferred_io_init(struct fb_info *info);
+extern int  fb_deferred_io_init(struct fb_info *info);
 extern void fb_deferred_io_open(struct fb_info *info,
 				struct inode *inode,
 				struct file *file);
-- 
2.36.0

[PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Thomas Zimmermann]

Store the per-page state for fbdev's deferred I/O in struct
fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
that have to be written back to video memory. Update all affected
drivers.

As with pages before, fbdev acquires a pageref when an mmaped page
of the framebuffer is being written to. It holds the pageref in a
list of all currently written pagerefs until it flushes the written
pages to video memory. Writeback occurs periodically. After writeback
fbdev releases all pagerefs and builds up a new dirty list until the
next writeback occurs.

Using pagerefs has a number of benefits.

For pages of the framebuffer, the deferred I/O code used struct
page.lru as an entry into the list of dirty pages. The lru field is
owned by the page cache, which makes deferred I/O incompatible with
some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
struct fb_deferred_io_pageref now provides an entry into a list of
dirty framebuffer pages, freeing lru for use with the page cache.

Drivers also assumed that struct page.index is the page offset into
the framebuffer. This is not true for DRM buffers, which are located
at various offset within a mapped area. struct fb_deferred_io_pageref
explicitly stores an offset into the framebuffer. struct page.index
is now only the page offset into the mapped area.

These changes will allow DRM to use fbdev deferred I/O without an
intermediate shadow buffer.

v2:
	* minor fixes in commit message

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c        |   6 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c     |   5 +-
 drivers/staging/fbtft/fbtft-core.c     |   5 +-
 drivers/video/fbdev/broadsheetfb.c     |   5 +-
 drivers/video/fbdev/core/fb_defio.c    | 156 ++++++++++++++++---------
 drivers/video/fbdev/hyperv_fb.c        |   5 +-
 drivers/video/fbdev/metronomefb.c      |   5 +-
 drivers/video/fbdev/sh_mobile_lcdcfb.c |   6 +-
 drivers/video/fbdev/smscufx.c          |   5 +-
 drivers/video/fbdev/udlfb.c            |   5 +-
 drivers/video/fbdev/xen-fbfront.c      |   5 +-
 include/linux/fb.h                     |  11 +-
 12 files changed, 145 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d06ce0e92d66..dd1d72d58b35 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -717,13 +717,13 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
 			       struct list_head *pagelist)
 {
 	unsigned long start, end, min, max;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	struct drm_rect damage_area;
 
 	min = ULONG_MAX;
 	max = 0;
-	list_for_each_entry(page, pagelist, lru) {
-		start = page->index << PAGE_SHIFT;
+	list_for_each_entry(pageref, pagelist, list) {
+		start = pageref->offset;
 		end = start + PAGE_SIZE;
 		min = min(min, start);
 		max = max(max, end);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index 02a4a4fa3da3..db02e17e12b6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
 	struct vmw_fb_par *par = info->par;
 	unsigned long start, end, min, max;
 	unsigned long flags;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	int y1, y2;
 
 	min = ULONG_MAX;
 	max = 0;
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		start = page->index << PAGE_SHIFT;
 		end = start + PAGE_SIZE - 1;
 		min = min(min, start);
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index d4e14f7c9d57..e9662822e1ef 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -326,7 +326,7 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagelist)
 {
 	struct fbtft_par *par = info->par;
 	unsigned int dirty_lines_start, dirty_lines_end;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long index;
 	unsigned int y_low = 0, y_high = 0;
 	int count = 0;
@@ -340,7 +340,8 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagelist)
 	spin_unlock(&par->dirty_lock);
 
 	/* Mark display lines as dirty */
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		count++;
 		index = page->index << PAGE_SHIFT;
 		y_low = index / info->fix.line_length;
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index 528bc0902338..6afc6ef4cb5e 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -934,7 +934,7 @@ static void broadsheetfb_dpy_deferred_io(struct fb_info *info,
 {
 	u16 y1 = 0, h = 0;
 	int prev_index = -1;
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	int h_inc;
 	u16 yres = info->var.yres;
@@ -944,7 +944,8 @@ static void broadsheetfb_dpy_deferred_io(struct fb_info *info,
 	h_inc = DIV_ROUND_UP(PAGE_SIZE , xres);
 
 	/* walk the written page list and swizzle the data */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		if (prev_index < 0) {
 			/* just starting so assign first page */
 			y1 = (cur->index << PAGE_SHIFT) / xres;
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 6924d489a289..a03b9c64fc61 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
 	return page;
 }
 
+static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
+								 unsigned long offset,
+								 struct page *page)
+{
+	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct list_head *pos = &fbdefio->pagelist;
+	unsigned long pgoff = offset >> PAGE_SHIFT;
+	struct fb_deferred_io_pageref *pageref, *cur;
+
+	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
+		return NULL; /* incorrect allocation size */
+
+	/* 1:1 mapping between pageref and page offset */
+	pageref = &info->pagerefs[pgoff];
+
+	/*
+	 * This check is to catch the case where a new process could start
+	 * writing to the same page through a new PTE. This new access
+	 * can cause a call to .page_mkwrite even if the original process'
+	 * PTE is marked writable.
+	 */
+	if (!list_empty(&pageref->list))
+		goto pageref_already_added;
+
+	pageref->page = page;
+	pageref->offset = pgoff << PAGE_SHIFT;
+
+	if (unlikely(fbdefio->sort_pagelist)) {
+		/*
+		 * We loop through the list of pagerefs before adding, in
+		 * order to keep the pagerefs sorted. This has significant
+		 * overhead of O(n^2) with n being the number of written
+		 * pages. If possible, drivers should try to work with
+		 * unsorted page lists instead.
+		 */
+		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
+			if (cur > pageref)
+				break;
+		}
+		pos = &cur->list;
+	}
+
+	list_add_tail(&pageref->list, pos);
+
+pageref_already_added:
+	return pageref;
+}
+
+static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
+				       struct fb_info *info)
+{
+	list_del_init(&pageref->list);
+}
+
 /* this is to find and return the vmalloc-ed fb pages */
 static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
 {
@@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
 		printk(KERN_ERR "no mapping available\n");
 
 	BUG_ON(!page->mapping);
-	page->index = vmf->pgoff;
+	page->index = vmf->pgoff; /* for page_mkclean() */
 
 	vmf->page = page;
 	return 0;
@@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	struct page *page = vmf->page;
 	struct fb_info *info = vmf->vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct list_head *pos = &fbdefio->pagelist;
+	struct fb_deferred_io_pageref *pageref;
+	unsigned long offset;
+	vm_fault_t ret;
+
+	offset = (vmf->address - vmf->vma->vm_start);
 
 	/* this is a callback we get when userspace first tries to
 	write to the page. we schedule a workqueue. that workqueue
@@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
 		fbdefio->first_io(info);
 
+	pageref = fb_deferred_io_pageref_get(info, offset, page);
+	if (WARN_ON_ONCE(!pageref)) {
+		ret = VM_FAULT_OOM;
+		goto err_mutex_unlock;
+	}
+
 	/*
 	 * We want the page to remain locked from ->page_mkwrite until
 	 * the PTE is marked dirty to avoid page_mkclean() being called
@@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	 * Do this by locking the page here and informing the caller
 	 * about it with VM_FAULT_LOCKED.
 	 */
-	lock_page(page);
-
-	/*
-	 * This check is to catch the case where a new process could start
-	 * writing to the same page through a new PTE. This new access
-	 * can cause a call to .page_mkwrite even if the original process'
-	 * PTE is marked writable.
-	 *
-	 * TODO: The lru field is owned by the page cache; hence the name.
-	 *       We dequeue in fb_deferred_io_work() after flushing the
-	 *       page's content into video memory. Instead of lru, fbdefio
-	 *       should have it's own field.
-	 */
-	if (!list_empty(&page->lru))
-		goto page_already_added;
-
-	if (unlikely(fbdefio->sort_pagelist)) {
-		/*
-		 * We loop through the pagelist before adding in order to
-		 * keep the pagelist sorted. This has significant overhead
-		 * of O(n^2) with n being the number of written pages. If
-		 * possible, drivers should try to work with unsorted page
-		 * lists instead.
-		 */
-		struct page *cur;
-
-		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
-			if (cur->index > page->index)
-				break;
-		}
-		pos = &cur->lru;
-	}
-
-	list_add_tail(&page->lru, pos);
+	lock_page(pageref->page);
 
-page_already_added:
 	mutex_unlock(&fbdefio->lock);
 
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
 	return VM_FAULT_LOCKED;
+
+err_mutex_unlock:
+	mutex_unlock(&fbdefio->lock);
+	return ret;
 }
 
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
@@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
 {
-	struct fb_info *info = container_of(work, struct fb_info,
-						deferred_work.work);
-	struct list_head *node, *next;
-	struct page *cur;
+	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
+	struct fb_deferred_io_pageref *pageref, *next;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 
 	/* here we mkclean the pages, then do all deferred IO */
 	mutex_lock(&fbdefio->lock);
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		lock_page(cur);
 		page_mkclean(cur);
 		unlock_page(cur);
@@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
 	fbdefio->deferred_io(info, &fbdefio->pagelist);
 
 	/* clear the list */
-	list_for_each_safe(node, next, &fbdefio->pagelist) {
-		list_del_init(node);
-	}
+	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
+		fb_deferred_io_pageref_put(pageref, info);
+
 	mutex_unlock(&fbdefio->lock);
 }
 
-void fb_deferred_io_init(struct fb_info *info)
+int fb_deferred_io_init(struct fb_info *info)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct page *page;
-	unsigned int i;
+	struct fb_deferred_io_pageref *pagerefs;
+	unsigned long npagerefs, i;
+	int ret;
 
 	BUG_ON(!fbdefio);
+
+	if (WARN_ON(!info->fix.smem_len))
+		return -EINVAL;
+
 	mutex_init(&fbdefio->lock);
 	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
 	INIT_LIST_HEAD(&fbdefio->pagelist);
 	if (fbdefio->delay == 0) /* set a default of 1 s */
 		fbdefio->delay = HZ;
 
-	/* initialize all the page lists one time */
-	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
-		page = fb_deferred_io_page(info, i);
-		INIT_LIST_HEAD(&page->lru);
+	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
+
+	/* alloc a page ref for each page of the display memory */
+	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
+	if (!pagerefs) {
+		ret = -ENOMEM;
+		goto err;
 	}
+	for (i = 0; i < npagerefs; ++i)
+		INIT_LIST_HEAD(&pagerefs[i].list);
+	info->npagerefs = npagerefs;
+	info->pagerefs = pagerefs;
+
+	return 0;
+
+err:
+	mutex_destroy(&fbdefio->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_init);
 
@@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 		page->mapping = NULL;
 	}
 
+	kvfree(info->pagerefs);
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index d7f6abf827b9..51eb57ea68f7 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -424,7 +424,7 @@ static void synthvid_deferred_io(struct fb_info *p,
 				 struct list_head *pagelist)
 {
 	struct hvfb_par *par = p->par;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long start, end;
 	int y1, y2, miny, maxy;
 
@@ -437,7 +437,8 @@ static void synthvid_deferred_io(struct fb_info *p,
 	 * in synthvid_update function by clamping the y2
 	 * value to yres.
 	 */
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		start = page->index << PAGE_SHIFT;
 		end = start + PAGE_SIZE - 1;
 		y1 = start / p->fix.line_length;
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 2541f2fe065b..8352fa3f4cef 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -469,12 +469,13 @@ static void metronomefb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
 	u16 cksum;
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct metronomefb_par *par = info->par;
 
 	/* walk the written page list and swizzle the data */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 		cksum = metronomefb_dpy_update_page(par,
 					(cur->index << PAGE_SHIFT));
 		par->metromem_img_csum -= par->csum_table[cur->index];
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index 1dc5079e4518..7fcc85352033 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -440,13 +440,15 @@ static int sh_mobile_lcdc_sginit(struct fb_info *info,
 {
 	struct sh_mobile_lcdc_chan *ch = info->par;
 	unsigned int nr_pages_max = ch->fb_size >> PAGE_SHIFT;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	int nr_pages = 0;
 
 	sg_init_table(ch->sglist, nr_pages_max);
 
-	list_for_each_entry(page, pagelist, lru)
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		sg_set_page(&ch->sglist[nr_pages++], page, PAGE_SIZE, 0);
+	}
 
 	return nr_pages;
 }
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 9383f8d0914c..eb108f56ea04 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -958,7 +958,7 @@ static void ufx_ops_fillrect(struct fb_info *info,
 static void ufx_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct ufx_data *dev = info->par;
 
@@ -969,9 +969,10 @@ static void ufx_dpy_deferred_io(struct fb_info *info,
 		return;
 
 	/* walk the written page list and render each to device */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
 		/* create a rectangle of full screen width that encloses the
 		 * entire dirty framebuffer page */
+		struct page *cur = pageref->page;
 		const int x = 0;
 		const int width = dev->info->var.xres;
 		const int y = (cur->index << PAGE_SHIFT) / (width * 2);
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 9e5a33396ef9..8249ed8700d4 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -784,7 +784,7 @@ static void dlfb_ops_fillrect(struct fb_info *info,
 static void dlfb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
 {
-	struct page *cur;
+	struct fb_deferred_io_pageref *pageref;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct dlfb_data *dlfb = info->par;
 	struct urb *urb;
@@ -811,7 +811,8 @@ static void dlfb_dpy_deferred_io(struct fb_info *info,
 	cmd = urb->transfer_buffer;
 
 	/* walk the written page list and render each to device */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
+		struct page *cur = pageref->page;
 
 		if (dlfb_render_hline(dlfb, &urb, (char *) info->fix.smem_start,
 				  &cmd, cur->index << PAGE_SHIFT,
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 6c8846eba2fb..608fcde767d3 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -185,13 +185,14 @@ static void xenfb_deferred_io(struct fb_info *fb_info,
 			      struct list_head *pagelist)
 {
 	struct xenfb_info *info = fb_info->par;
-	struct page *page;
+	struct fb_deferred_io_pageref *pageref;
 	unsigned long beg, end;
 	int y1, y2, miny, maxy;
 
 	miny = INT_MAX;
 	maxy = 0;
-	list_for_each_entry(page, pagelist, lru) {
+	list_for_each_entry(pageref, pagelist, list) {
+		struct page *page = pageref->page;
 		beg = page->index << PAGE_SHIFT;
 		end = beg + PAGE_SIZE - 1;
 		y1 = beg / fb_info->fix.line_length;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f95da1af9ff6..a332590c0fae 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -201,6 +201,13 @@ struct fb_pixmap {
 };
 
 #ifdef CONFIG_FB_DEFERRED_IO
+struct fb_deferred_io_pageref {
+	struct page *page;
+	unsigned long offset;
+	/* private */
+	struct list_head list;
+};
+
 struct fb_deferred_io {
 	/* delay between mkwrite and deferred handler */
 	unsigned long delay;
@@ -468,6 +475,8 @@ struct fb_info {
 #endif
 #ifdef CONFIG_FB_DEFERRED_IO
 	struct delayed_work deferred_work;
+	unsigned long npagerefs;
+	struct fb_deferred_io_pageref *pagerefs;
 	struct fb_deferred_io *fbdefio;
 #endif
 
@@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
 
 /* drivers/video/fb_defio.c */
 int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
-extern void fb_deferred_io_init(struct fb_info *info);
+extern int  fb_deferred_io_init(struct fb_info *info);
 extern void fb_deferred_io_open(struct fb_info *info,
 				struct inode *inode,
 				struct file *file);
-- 
2.36.0

[PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

Refactor the page-write handler for deferred I/O. Drivers use the
function to let fbdev track written pages of mmap'ed framebuffer
memory.

v2:
	* don't export the helper until we have an external caller

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index a03b9c64fc61..214581ce5840 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
 
-/* vm_ops->page_mkwrite handler */
-static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+/*
+ * Adds a page to the dirty list. Requires caller to hold
+ * struct fb_deferred_io.lock. Call this from struct
+ * vm_operations_struct.page_mkwrite.
+ */
+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
+					      struct page *page)
 {
-	struct page *page = vmf->page;
-	struct fb_info *info = vmf->vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct fb_deferred_io_pageref *pageref;
-	unsigned long offset;
 	vm_fault_t ret;
 
-	offset = (vmf->address - vmf->vma->vm_start);
-
-	/* this is a callback we get when userspace first tries to
-	write to the page. we schedule a workqueue. that workqueue
-	will eventually mkclean the touched pages and execute the
-	deferred framebuffer IO. then if userspace touches a page
-	again, we repeat the same scheme */
-
-	file_update_time(vmf->vma->vm_file);
-
-	/* protect against the workqueue changing the page list */
-	mutex_lock(&fbdefio->lock);
-
 	/* first write in this cycle, notify the driver */
 	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
 		fbdefio->first_io(info);
@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	 */
 	lock_page(pageref->page);
 
-	mutex_unlock(&fbdefio->lock);
-
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
 	return VM_FAULT_LOCKED;
@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	return ret;
 }
 
+/*
+ * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
+ * @fb_info: The fbdev info structure
+ * @vmf: The VM fault
+ *
+ * This is a callback we get when userspace first tries to
+ * write to the page. We schedule a workqueue. That workqueue
+ * will eventually mkclean the touched pages and execute the
+ * deferred framebuffer IO. Then if userspace touches a page
+ * again, we repeat the same scheme.
+ *
+ * Returns:
+ * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
+ */
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
+{
+	struct page *page = vmf->page;
+	struct fb_deferred_io *fbdefio = info->fbdefio;
+	unsigned long offset;
+	vm_fault_t ret;
+
+	offset = (vmf->address - vmf->vma->vm_start);
+
+	file_update_time(vmf->vma->vm_file);
+
+	/* protect against the workqueue changing the page list */
+	mutex_lock(&fbdefio->lock);
+	ret = __fb_deferred_io_track_page(info, offset, page);
+	mutex_unlock(&fbdefio->lock);
+
+	return ret;
+}
+
+/* vm_ops->page_mkwrite handler */
+static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+{
+	struct fb_info *info = vmf->vma->vm_private_data;
+
+	return fb_deferred_io_page_mkwrite(info, vmf);
+}
+
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
 	.fault		= fb_deferred_io_fault,
 	.page_mkwrite	= fb_deferred_io_mkwrite,
-- 
2.36.0

[PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

Refactor the page-write handler for deferred I/O. Drivers use the
function to let fbdev track written pages of mmap'ed framebuffer
memory.

v2:
	* don't export the helper until we have an external caller

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index a03b9c64fc61..214581ce5840 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
 
-/* vm_ops->page_mkwrite handler */
-static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+/*
+ * Adds a page to the dirty list. Requires caller to hold
+ * struct fb_deferred_io.lock. Call this from struct
+ * vm_operations_struct.page_mkwrite.
+ */
+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
+					      struct page *page)
 {
-	struct page *page = vmf->page;
-	struct fb_info *info = vmf->vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct fb_deferred_io_pageref *pageref;
-	unsigned long offset;
 	vm_fault_t ret;
 
-	offset = (vmf->address - vmf->vma->vm_start);
-
-	/* this is a callback we get when userspace first tries to
-	write to the page. we schedule a workqueue. that workqueue
-	will eventually mkclean the touched pages and execute the
-	deferred framebuffer IO. then if userspace touches a page
-	again, we repeat the same scheme */
-
-	file_update_time(vmf->vma->vm_file);
-
-	/* protect against the workqueue changing the page list */
-	mutex_lock(&fbdefio->lock);
-
 	/* first write in this cycle, notify the driver */
 	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
 		fbdefio->first_io(info);
@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	 */
 	lock_page(pageref->page);
 
-	mutex_unlock(&fbdefio->lock);
-
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
 	return VM_FAULT_LOCKED;
@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	return ret;
 }
 
+/*
+ * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
+ * @fb_info: The fbdev info structure
+ * @vmf: The VM fault
+ *
+ * This is a callback we get when userspace first tries to
+ * write to the page. We schedule a workqueue. That workqueue
+ * will eventually mkclean the touched pages and execute the
+ * deferred framebuffer IO. Then if userspace touches a page
+ * again, we repeat the same scheme.
+ *
+ * Returns:
+ * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
+ */
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
+{
+	struct page *page = vmf->page;
+	struct fb_deferred_io *fbdefio = info->fbdefio;
+	unsigned long offset;
+	vm_fault_t ret;
+
+	offset = (vmf->address - vmf->vma->vm_start);
+
+	file_update_time(vmf->vma->vm_file);
+
+	/* protect against the workqueue changing the page list */
+	mutex_lock(&fbdefio->lock);
+	ret = __fb_deferred_io_track_page(info, offset, page);
+	mutex_unlock(&fbdefio->lock);
+
+	return ret;
+}
+
+/* vm_ops->page_mkwrite handler */
+static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+{
+	struct fb_info *info = vmf->vma->vm_private_data;
+
+	return fb_deferred_io_page_mkwrite(info, vmf);
+}
+
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
 	.fault		= fb_deferred_io_fault,
 	.page_mkwrite	= fb_deferred_io_mkwrite,
-- 
2.36.0

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[kernel test robot]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on 0e7deff6446a4ba2c75f499a0bfa80cd6a15c129]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
base:   0e7deff6446a4ba2c75f499a0bfa80cd6a15c129
config: parisc-randconfig-r011-20220425 (https://download.01.org/0day-ci/archive/20220425/202204252303.Xu2N4l6Y-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
        git checkout 183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:11,
                    from include/linux/kernel.h:29,
                    from include/linux/cpumask.h:10,
                    from include/linux/mm_types_task.h:14,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/video/fbdev/core/fbmem.c:14:
   drivers/video/fbdev/core/fbmem.c: In function 'fb_mmap':
>> drivers/video/fbdev/core/fbmem.c:1362:42: error: 'struct fb_info' has no member named 'fbdefio'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |                                          ^~
   include/linux/once_lite.h:15:41: note: in definition of macro 'DO_ONCE_LITE_IF'
      15 |                 bool __ret_do_once = !!(condition);                     \
         |                                         ^~~~~~~~~
   include/linux/dev_printk.h:274:9: note: in expansion of macro 'WARN_ONCE'
     274 |         WARN_ONCE(condition, "%s %s: " format, \
         |         ^~~~~~~~~
   drivers/video/fbdev/core/fbmem.c:1362:13: note: in expansion of macro 'dev_WARN_ONCE'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |             ^~~~~~~~~~~~~


vim +1362 drivers/video/fbdev/core/fbmem.c

  1332	
  1333	static int
  1334	fb_mmap(struct file *file, struct vm_area_struct * vma)
  1335	{
  1336		struct fb_info *info = file_fb_info(file);
  1337		unsigned long mmio_pgoff;
  1338		unsigned long start;
  1339		u32 len;
  1340	
  1341		if (!info)
  1342			return -ENODEV;
  1343		mutex_lock(&info->mm_lock);
  1344	
  1345		if (info->fbops->fb_mmap) {
  1346			int res;
  1347	
  1348			/*
  1349			 * The framebuffer needs to be accessed decrypted, be sure
  1350			 * SME protection is removed ahead of the call
  1351			 */
  1352			vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
  1353			res = info->fbops->fb_mmap(info, vma);
  1354			mutex_unlock(&info->mm_lock);
  1355			return res;
  1356		}
  1357	
  1358		/*
  1359		 * FB deferred I/O wants you to handle mmap in your drivers. At a
  1360		 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
  1361		 */
> 1362		if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
  1363			return -ENODEV;
  1364	
  1365		/*
  1366		 * Ugh. This can be either the frame buffer mapping, or
  1367		 * if pgoff points past it, the mmio mapping.
  1368		 */
  1369		start = info->fix.smem_start;
  1370		len = info->fix.smem_len;
  1371		mmio_pgoff = PAGE_ALIGN((start & ~PAGE_MASK) + len) >> PAGE_SHIFT;
  1372		if (vma->vm_pgoff >= mmio_pgoff) {
  1373			if (info->var.accel_flags) {
  1374				mutex_unlock(&info->mm_lock);
  1375				return -EINVAL;
  1376			}
  1377	
  1378			vma->vm_pgoff -= mmio_pgoff;
  1379			start = info->fix.mmio_start;
  1380			len = info->fix.mmio_len;
  1381		}
  1382		mutex_unlock(&info->mm_lock);
  1383	
  1384		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
  1385		fb_pgprotect(file, vma, start);
  1386	
  1387		return vm_iomap_memory(vma, start, len);
  1388	}
  1389	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[kernel test robot]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on 0e7deff6446a4ba2c75f499a0bfa80cd6a15c129]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
base:   0e7deff6446a4ba2c75f499a0bfa80cd6a15c129
config: parisc-randconfig-r011-20220425 (https://download.01.org/0day-ci/archive/20220425/202204252303.Xu2N4l6Y-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Decouple-deferred-I-O-from-struct-page/20220425-192955
        git checkout 183267de16bfe1cd6ec119bcfdaf95e3706bf87e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:11,
                    from include/linux/kernel.h:29,
                    from include/linux/cpumask.h:10,
                    from include/linux/mm_types_task.h:14,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/video/fbdev/core/fbmem.c:14:
   drivers/video/fbdev/core/fbmem.c: In function 'fb_mmap':
>> drivers/video/fbdev/core/fbmem.c:1362:42: error: 'struct fb_info' has no member named 'fbdefio'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |                                          ^~
   include/linux/once_lite.h:15:41: note: in definition of macro 'DO_ONCE_LITE_IF'
      15 |                 bool __ret_do_once = !!(condition);                     \
         |                                         ^~~~~~~~~
   include/linux/dev_printk.h:274:9: note: in expansion of macro 'WARN_ONCE'
     274 |         WARN_ONCE(condition, "%s %s: " format, \
         |         ^~~~~~~~~
   drivers/video/fbdev/core/fbmem.c:1362:13: note: in expansion of macro 'dev_WARN_ONCE'
    1362 |         if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
         |             ^~~~~~~~~~~~~


vim +1362 drivers/video/fbdev/core/fbmem.c

  1332	
  1333	static int
  1334	fb_mmap(struct file *file, struct vm_area_struct * vma)
  1335	{
  1336		struct fb_info *info = file_fb_info(file);
  1337		unsigned long mmio_pgoff;
  1338		unsigned long start;
  1339		u32 len;
  1340	
  1341		if (!info)
  1342			return -ENODEV;
  1343		mutex_lock(&info->mm_lock);
  1344	
  1345		if (info->fbops->fb_mmap) {
  1346			int res;
  1347	
  1348			/*
  1349			 * The framebuffer needs to be accessed decrypted, be sure
  1350			 * SME protection is removed ahead of the call
  1351			 */
  1352			vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
  1353			res = info->fbops->fb_mmap(info, vma);
  1354			mutex_unlock(&info->mm_lock);
  1355			return res;
  1356		}
  1357	
  1358		/*
  1359		 * FB deferred I/O wants you to handle mmap in your drivers. At a
  1360		 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
  1361		 */
> 1362		if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
  1363			return -ENODEV;
  1364	
  1365		/*
  1366		 * Ugh. This can be either the frame buffer mapping, or
  1367		 * if pgoff points past it, the mmio mapping.
  1368		 */
  1369		start = info->fix.smem_start;
  1370		len = info->fix.smem_len;
  1371		mmio_pgoff = PAGE_ALIGN((start & ~PAGE_MASK) + len) >> PAGE_SHIFT;
  1372		if (vma->vm_pgoff >= mmio_pgoff) {
  1373			if (info->var.accel_flags) {
  1374				mutex_unlock(&info->mm_lock);
  1375				return -EINVAL;
  1376			}
  1377	
  1378			vma->vm_pgoff -= mmio_pgoff;
  1379			start = info->fix.mmio_start;
  1380			len = info->fix.mmio_len;
  1381		}
  1382		mutex_unlock(&info->mm_lock);
  1383	
  1384		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
  1385		fb_pgprotect(file, vma, start);
  1386	
  1387		return vm_iomap_memory(vma, start, len);
  1388	}
  1389	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Sam Ravnborg]

Hi Thomas,

> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 6aaf6d0abf39..6924d489a289 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  	vma->vm_private_data = info;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>  
>  /* workqueue callback */
>  static void fb_deferred_io_work(struct work_struct *work)
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 84427470367b..52440e3f8f69 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1334,7 +1334,6 @@ static int
>  fb_mmap(struct file *file, struct vm_area_struct * vma)
>  {
>  	struct fb_info *info = file_fb_info(file);
> -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
>  	unsigned long mmio_pgoff;
>  	unsigned long start;
>  	u32 len;
> @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>  		return -ENODEV;
>  	mutex_lock(&info->mm_lock);
>  
> -	fb_mmap_fn = info->fbops->fb_mmap;
> -
> -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> -	if (info->fbdefio)
> -		fb_mmap_fn = fb_deferred_io_mmap;
> -#endif
> -
> -	if (fb_mmap_fn) {
> +	if (info->fbops->fb_mmap) {
>  		int res;
>  
>  		/*
> @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>  		 * SME protection is removed ahead of the call
>  		 */
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> -		res = fb_mmap_fn(info, vma);
> +		res = info->fbops->fb_mmap(info, vma);
>  		mutex_unlock(&info->mm_lock);
>  		return res;
>  	}
>  
> +	/*
> +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
> +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> +	 */
> +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
> +		return -ENODEV;
> +

If not configured - then why not just call fb_deferred_io_mmap(), as
this seems to be the default implementation anyway.
Then drivers that needs it can override - and the rest fallback to the
default.

	Sam

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Sam Ravnborg]

Hi Thomas,

> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 6aaf6d0abf39..6924d489a289 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  	vma->vm_private_data = info;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>  
>  /* workqueue callback */
>  static void fb_deferred_io_work(struct work_struct *work)
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 84427470367b..52440e3f8f69 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1334,7 +1334,6 @@ static int
>  fb_mmap(struct file *file, struct vm_area_struct * vma)
>  {
>  	struct fb_info *info = file_fb_info(file);
> -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
>  	unsigned long mmio_pgoff;
>  	unsigned long start;
>  	u32 len;
> @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>  		return -ENODEV;
>  	mutex_lock(&info->mm_lock);
>  
> -	fb_mmap_fn = info->fbops->fb_mmap;
> -
> -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> -	if (info->fbdefio)
> -		fb_mmap_fn = fb_deferred_io_mmap;
> -#endif
> -
> -	if (fb_mmap_fn) {
> +	if (info->fbops->fb_mmap) {
>  		int res;
>  
>  		/*
> @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>  		 * SME protection is removed ahead of the call
>  		 */
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> -		res = fb_mmap_fn(info, vma);
> +		res = info->fbops->fb_mmap(info, vma);
>  		mutex_unlock(&info->mm_lock);
>  		return res;
>  	}
>  
> +	/*
> +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
> +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> +	 */
> +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
> +		return -ENODEV;
> +

If not configured - then why not just call fb_deferred_io_mmap(), as
this seems to be the default implementation anyway.
Then drivers that needs it can override - and the rest fallback to the
default.

	Sam

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Sam Ravnborg]

Hi Thomas,

On Mon, Apr 25, 2022 at 07:26:32PM +0200, Sam Ravnborg wrote:
> Hi Thomas,
> 
> > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > index 6aaf6d0abf39..6924d489a289 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >  	vma->vm_private_data = info;
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
> >  
> >  /* workqueue callback */
> >  static void fb_deferred_io_work(struct work_struct *work)
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index 84427470367b..52440e3f8f69 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1334,7 +1334,6 @@ static int
> >  fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  {
> >  	struct fb_info *info = file_fb_info(file);
> > -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> >  	unsigned long mmio_pgoff;
> >  	unsigned long start;
> >  	u32 len;
> > @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		return -ENODEV;
> >  	mutex_lock(&info->mm_lock);
> >  
> > -	fb_mmap_fn = info->fbops->fb_mmap;
> > -
> > -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> > -	if (info->fbdefio)
> > -		fb_mmap_fn = fb_deferred_io_mmap;
> > -#endif
> > -
> > -	if (fb_mmap_fn) {
> > +	if (info->fbops->fb_mmap) {
> >  		int res;
> >  
> >  		/*
> > @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		 * SME protection is removed ahead of the call
> >  		 */
> >  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > -		res = fb_mmap_fn(info, vma);
> > +		res = info->fbops->fb_mmap(info, vma);
> >  		mutex_unlock(&info->mm_lock);
> >  		return res;
> >  	}
> >  
> > +	/*
> > +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
> > +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> > +	 */
> > +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
> > +		return -ENODEV;
> > +
> 
> If not configured - then why not just call fb_deferred_io_mmap(), as
> this seems to be the default implementation anyway.
> Then drivers that needs it can override - and the rest fallback to the
> default.

Just to be clear - I already read:
"
Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.
"

But this does not help me understand why we need to explicit do what
could be a simple default implementation.
Chances are that I am stupid and it is obvious..

	Sam

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Sam Ravnborg]

Hi Thomas,

On Mon, Apr 25, 2022 at 07:26:32PM +0200, Sam Ravnborg wrote:
> Hi Thomas,
> 
> > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > index 6aaf6d0abf39..6924d489a289 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >  	vma->vm_private_data = info;
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
> >  
> >  /* workqueue callback */
> >  static void fb_deferred_io_work(struct work_struct *work)
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index 84427470367b..52440e3f8f69 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1334,7 +1334,6 @@ static int
> >  fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  {
> >  	struct fb_info *info = file_fb_info(file);
> > -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> >  	unsigned long mmio_pgoff;
> >  	unsigned long start;
> >  	u32 len;
> > @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		return -ENODEV;
> >  	mutex_lock(&info->mm_lock);
> >  
> > -	fb_mmap_fn = info->fbops->fb_mmap;
> > -
> > -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> > -	if (info->fbdefio)
> > -		fb_mmap_fn = fb_deferred_io_mmap;
> > -#endif
> > -
> > -	if (fb_mmap_fn) {
> > +	if (info->fbops->fb_mmap) {
> >  		int res;
> >  
> >  		/*
> > @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> >  		 * SME protection is removed ahead of the call
> >  		 */
> >  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > -		res = fb_mmap_fn(info, vma);
> > +		res = info->fbops->fb_mmap(info, vma);
> >  		mutex_unlock(&info->mm_lock);
> >  		return res;
> >  	}
> >  
> > +	/*
> > +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
> > +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> > +	 */
> > +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
> > +		return -ENODEV;
> > +
> 
> If not configured - then why not just call fb_deferred_io_mmap(), as
> this seems to be the default implementation anyway.
> Then drivers that needs it can override - and the rest fallback to the
> default.

Just to be clear - I already read:
"
Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.
"

But this does not help me understand why we need to explicit do what
could be a simple default implementation.
Chances are that I am stupid and it is obvious..

	Sam

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Sam Ravnborg]

Hi Thomas,

a little ramblings below. Just my thoughts while trying to understand
the code - especially since I looked at it before.

	Sam

On Mon, Apr 25, 2022 at 01:27:50PM +0200, Thomas Zimmermann wrote:
> Store the per-page state for fbdev's deferred I/O in struct
> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
> that have to be written back to video memory. Update all affected
> drivers.
> 
> As with pages before, fbdev acquires a pageref when an mmaped page
> of the framebuffer is being written to. It holds the pageref in a
> list of all currently written pagerefs until it flushes the written
> pages to video memory. Writeback occurs periodically. After writeback
> fbdev releases all pagerefs and builds up a new dirty list until the
> next writeback occurs.
> 
> Using pagerefs has a number of benefits.
> 
> For pages of the framebuffer, the deferred I/O code used struct
> page.lru as an entry into the list of dirty pages. The lru field is
> owned by the page cache, which makes deferred I/O incompatible with
> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
> struct fb_deferred_io_pageref now provides an entry into a list of
> dirty framebuffer pages, freeing lru for use with the page cache.
> 
> Drivers also assumed that struct page.index is the page offset into
> the framebuffer. This is not true for DRM buffers, which are located
> at various offset within a mapped area. struct fb_deferred_io_pageref
> explicitly stores an offset into the framebuffer. struct page.index
> is now only the page offset into the mapped area.
> 
> These changes will allow DRM to use fbdev deferred I/O without an
> intermediate shadow buffer.
> 
> v2:
> 	* minor fixes in commit message
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---


> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index 02a4a4fa3da3..db02e17e12b6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
>  	struct vmw_fb_par *par = info->par;
>  	unsigned long start, end, min, max;
>  	unsigned long flags;
> -	struct page *page;
> +	struct fb_deferred_io_pageref *pageref;
>  	int y1, y2;
>  
>  	min = ULONG_MAX;
>  	max = 0;
> -	list_for_each_entry(page, pagelist, lru) {
> +	list_for_each_entry(pageref, pagelist, list) {
> +		struct page *page = pageref->page;
>  		start = page->index << PAGE_SHIFT;
>  		end = start + PAGE_SIZE - 1;
>  		min = min(min, start);

This is the same change in all deferred_io drivers like above.
We now traverse pageref->list where pagelist points to the head.

It took me a while to understand that pagelist points to a list of
fb_deferred_io_pageref and not a list of pages.
I had been helped, had this been renamed to s/pagelist/pagereflist/.

If you see no point in this then just ignore my comment, I have not
stared at kernel code for a while and is thus easy to confuse.

> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 6924d489a289..a03b9c64fc61 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
>  	return page;
>  }
>  
> +static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
> +								 unsigned long offset,
> +								 struct page *page)
> +{
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	struct list_head *pos = &fbdefio->pagelist;
> +	unsigned long pgoff = offset >> PAGE_SHIFT;
> +	struct fb_deferred_io_pageref *pageref, *cur;
> +
> +	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
> +		return NULL; /* incorrect allocation size */
> +
> +	/* 1:1 mapping between pageref and page offset */
> +	pageref = &info->pagerefs[pgoff];
> +
> +	/*
> +	 * This check is to catch the case where a new process could start
> +	 * writing to the same page through a new PTE. This new access
> +	 * can cause a call to .page_mkwrite even if the original process'
> +	 * PTE is marked writable.
> +	 */
> +	if (!list_empty(&pageref->list))
> +		goto pageref_already_added;
> +
> +	pageref->page = page;
> +	pageref->offset = pgoff << PAGE_SHIFT;
> +
> +	if (unlikely(fbdefio->sort_pagelist)) {
> +		/*
> +		 * We loop through the list of pagerefs before adding, in
> +		 * order to keep the pagerefs sorted. This has significant
> +		 * overhead of O(n^2) with n being the number of written
> +		 * pages. If possible, drivers should try to work with
> +		 * unsorted page lists instead.
> +		 */
> +		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
> +			if (cur > pageref)
> +				break;
> +		}
> +		pos = &cur->list;
> +	}
> +
> +	list_add_tail(&pageref->list, pos);
> +
> +pageref_already_added:
> +	return pageref;
> +}
> +
> +static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
> +				       struct fb_info *info)
> +{
> +	list_del_init(&pageref->list);
> +}
> +
>  /* this is to find and return the vmalloc-ed fb pages */
>  static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>  {
> @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>  		printk(KERN_ERR "no mapping available\n");
>  
>  	BUG_ON(!page->mapping);
> -	page->index = vmf->pgoff;
> +	page->index = vmf->pgoff; /* for page_mkclean() */
>  
>  	vmf->page = page;
>  	return 0;
> @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	struct page *page = vmf->page;
>  	struct fb_info *info = vmf->vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> -	struct list_head *pos = &fbdefio->pagelist;
> +	struct fb_deferred_io_pageref *pageref;
> +	unsigned long offset;
> +	vm_fault_t ret;
> +
> +	offset = (vmf->address - vmf->vma->vm_start);
>  
>  	/* this is a callback we get when userspace first tries to
>  	write to the page. we schedule a workqueue. that workqueue
> @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>  		fbdefio->first_io(info);
>  
> +	pageref = fb_deferred_io_pageref_get(info, offset, page);
Compared to the old code we now do all the sorting and stuff without
the page locked, which seem like a big change.


> +	if (WARN_ON_ONCE(!pageref)) {
> +		ret = VM_FAULT_OOM;
> +		goto err_mutex_unlock;
> +	}
> +
>  	/*
>  	 * We want the page to remain locked from ->page_mkwrite until
>  	 * the PTE is marked dirty to avoid page_mkclean() being called
> @@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	 * Do this by locking the page here and informing the caller
>  	 * about it with VM_FAULT_LOCKED.
>  	 */
> -	lock_page(page);
> -
> -	/*
> -	 * This check is to catch the case where a new process could start
> -	 * writing to the same page through a new PTE. This new access
> -	 * can cause a call to .page_mkwrite even if the original process'
> -	 * PTE is marked writable.
> -	 *
> -	 * TODO: The lru field is owned by the page cache; hence the name.
> -	 *       We dequeue in fb_deferred_io_work() after flushing the
> -	 *       page's content into video memory. Instead of lru, fbdefio
> -	 *       should have it's own field.
> -	 */
> -	if (!list_empty(&page->lru))
> -		goto page_already_added;
> -
> -	if (unlikely(fbdefio->sort_pagelist)) {
> -		/*
> -		 * We loop through the pagelist before adding in order to
> -		 * keep the pagelist sorted. This has significant overhead
> -		 * of O(n^2) with n being the number of written pages. If
> -		 * possible, drivers should try to work with unsorted page
> -		 * lists instead.
> -		 */
> -		struct page *cur;
> -
> -		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> -			if (cur->index > page->index)
> -				break;
> -		}
> -		pos = &cur->lru;
> -	}
> -
> -	list_add_tail(&page->lru, pos);
> +	lock_page(pageref->page);
>  
> -page_already_added:
>  	mutex_unlock(&fbdefio->lock);
>  
>  	/* come back after delay to process the deferred IO */
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>  	return VM_FAULT_LOCKED;
> +
> +err_mutex_unlock:
> +	mutex_unlock(&fbdefio->lock);
> +	return ret;
>  }
>  
>  static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> @@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>  /* workqueue callback */
>  static void fb_deferred_io_work(struct work_struct *work)
>  {
> -	struct fb_info *info = container_of(work, struct fb_info,
> -						deferred_work.work);
> -	struct list_head *node, *next;
> -	struct page *cur;
> +	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
> +	struct fb_deferred_io_pageref *pageref, *next;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
>  
>  	/* here we mkclean the pages, then do all deferred IO */
>  	mutex_lock(&fbdefio->lock);
> -	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> +	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
> +		struct page *cur = pageref->page;
>  		lock_page(cur);
>  		page_mkclean(cur);
>  		unlock_page(cur);
> @@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
>  	fbdefio->deferred_io(info, &fbdefio->pagelist);
>  
>  	/* clear the list */
> -	list_for_each_safe(node, next, &fbdefio->pagelist) {
> -		list_del_init(node);
> -	}
> +	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
> +		fb_deferred_io_pageref_put(pageref, info);
> +
>  	mutex_unlock(&fbdefio->lock);
>  }
>  
> -void fb_deferred_io_init(struct fb_info *info)
> +int fb_deferred_io_init(struct fb_info *info)
>  {
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> -	struct page *page;
> -	unsigned int i;
> +	struct fb_deferred_io_pageref *pagerefs;
> +	unsigned long npagerefs, i;
> +	int ret;
>  
>  	BUG_ON(!fbdefio);
> +
> +	if (WARN_ON(!info->fix.smem_len))
> +		return -EINVAL;
> +
>  	mutex_init(&fbdefio->lock);
>  	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
>  	INIT_LIST_HEAD(&fbdefio->pagelist);
>  	if (fbdefio->delay == 0) /* set a default of 1 s */
>  		fbdefio->delay = HZ;
>  
> -	/* initialize all the page lists one time */
> -	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
> -		page = fb_deferred_io_page(info, i);
> -		INIT_LIST_HEAD(&page->lru);
> +	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
> +
> +	/* alloc a page ref for each page of the display memory */
> +	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
> +	if (!pagerefs) {
> +		ret = -ENOMEM;
> +		goto err;
>  	}
> +	for (i = 0; i < npagerefs; ++i)
> +		INIT_LIST_HEAD(&pagerefs[i].list);
> +	info->npagerefs = npagerefs;
> +	info->pagerefs = pagerefs;
> +
> +	return 0;
> +
> +err:
> +	mutex_destroy(&fbdefio->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>  
> @@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>  		page->mapping = NULL;
>  	}
>  
> +	kvfree(info->pagerefs);
>  	mutex_destroy(&fbdefio->lock);
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);

> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index f95da1af9ff6..a332590c0fae 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -201,6 +201,13 @@ struct fb_pixmap {
>  };
>  
>  #ifdef CONFIG_FB_DEFERRED_IO
> +struct fb_deferred_io_pageref {
> +	struct page *page;
> +	unsigned long offset;
> +	/* private */
> +	struct list_head list;
This is the list of pages - so this could be named pagelist...

> +};
> +
>  struct fb_deferred_io {
>  	/* delay between mkwrite and deferred handler */
>  	unsigned long delay;
> @@ -468,6 +475,8 @@ struct fb_info {
>  #endif
>  #ifdef CONFIG_FB_DEFERRED_IO
>  	struct delayed_work deferred_work;
> +	unsigned long npagerefs;
> +	struct fb_deferred_io_pageref *pagerefs;
>  	struct fb_deferred_io *fbdefio;
>  #endif
>  
> @@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
>  
>  /* drivers/video/fb_defio.c */
>  int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> -extern void fb_deferred_io_init(struct fb_info *info);
> +extern int  fb_deferred_io_init(struct fb_info *info);
>  extern void fb_deferred_io_open(struct fb_info *info,
>  				struct inode *inode,
>  				struct file *file);
> -- 
> 2.36.0

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Sam Ravnborg]

Hi Thomas,

a little ramblings below. Just my thoughts while trying to understand
the code - especially since I looked at it before.

	Sam

On Mon, Apr 25, 2022 at 01:27:50PM +0200, Thomas Zimmermann wrote:
> Store the per-page state for fbdev's deferred I/O in struct
> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
> that have to be written back to video memory. Update all affected
> drivers.
> 
> As with pages before, fbdev acquires a pageref when an mmaped page
> of the framebuffer is being written to. It holds the pageref in a
> list of all currently written pagerefs until it flushes the written
> pages to video memory. Writeback occurs periodically. After writeback
> fbdev releases all pagerefs and builds up a new dirty list until the
> next writeback occurs.
> 
> Using pagerefs has a number of benefits.
> 
> For pages of the framebuffer, the deferred I/O code used struct
> page.lru as an entry into the list of dirty pages. The lru field is
> owned by the page cache, which makes deferred I/O incompatible with
> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
> struct fb_deferred_io_pageref now provides an entry into a list of
> dirty framebuffer pages, freeing lru for use with the page cache.
> 
> Drivers also assumed that struct page.index is the page offset into
> the framebuffer. This is not true for DRM buffers, which are located
> at various offset within a mapped area. struct fb_deferred_io_pageref
> explicitly stores an offset into the framebuffer. struct page.index
> is now only the page offset into the mapped area.
> 
> These changes will allow DRM to use fbdev deferred I/O without an
> intermediate shadow buffer.
> 
> v2:
> 	* minor fixes in commit message
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---


> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index 02a4a4fa3da3..db02e17e12b6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
>  	struct vmw_fb_par *par = info->par;
>  	unsigned long start, end, min, max;
>  	unsigned long flags;
> -	struct page *page;
> +	struct fb_deferred_io_pageref *pageref;
>  	int y1, y2;
>  
>  	min = ULONG_MAX;
>  	max = 0;
> -	list_for_each_entry(page, pagelist, lru) {
> +	list_for_each_entry(pageref, pagelist, list) {
> +		struct page *page = pageref->page;
>  		start = page->index << PAGE_SHIFT;
>  		end = start + PAGE_SIZE - 1;
>  		min = min(min, start);

This is the same change in all deferred_io drivers like above.
We now traverse pageref->list where pagelist points to the head.

It took me a while to understand that pagelist points to a list of
fb_deferred_io_pageref and not a list of pages.
I had been helped, had this been renamed to s/pagelist/pagereflist/.

If you see no point in this then just ignore my comment, I have not
stared at kernel code for a while and is thus easy to confuse.

> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 6924d489a289..a03b9c64fc61 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
>  	return page;
>  }
>  
> +static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
> +								 unsigned long offset,
> +								 struct page *page)
> +{
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	struct list_head *pos = &fbdefio->pagelist;
> +	unsigned long pgoff = offset >> PAGE_SHIFT;
> +	struct fb_deferred_io_pageref *pageref, *cur;
> +
> +	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
> +		return NULL; /* incorrect allocation size */
> +
> +	/* 1:1 mapping between pageref and page offset */
> +	pageref = &info->pagerefs[pgoff];
> +
> +	/*
> +	 * This check is to catch the case where a new process could start
> +	 * writing to the same page through a new PTE. This new access
> +	 * can cause a call to .page_mkwrite even if the original process'
> +	 * PTE is marked writable.
> +	 */
> +	if (!list_empty(&pageref->list))
> +		goto pageref_already_added;
> +
> +	pageref->page = page;
> +	pageref->offset = pgoff << PAGE_SHIFT;
> +
> +	if (unlikely(fbdefio->sort_pagelist)) {
> +		/*
> +		 * We loop through the list of pagerefs before adding, in
> +		 * order to keep the pagerefs sorted. This has significant
> +		 * overhead of O(n^2) with n being the number of written
> +		 * pages. If possible, drivers should try to work with
> +		 * unsorted page lists instead.
> +		 */
> +		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
> +			if (cur > pageref)
> +				break;
> +		}
> +		pos = &cur->list;
> +	}
> +
> +	list_add_tail(&pageref->list, pos);
> +
> +pageref_already_added:
> +	return pageref;
> +}
> +
> +static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
> +				       struct fb_info *info)
> +{
> +	list_del_init(&pageref->list);
> +}
> +
>  /* this is to find and return the vmalloc-ed fb pages */
>  static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>  {
> @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>  		printk(KERN_ERR "no mapping available\n");
>  
>  	BUG_ON(!page->mapping);
> -	page->index = vmf->pgoff;
> +	page->index = vmf->pgoff; /* for page_mkclean() */
>  
>  	vmf->page = page;
>  	return 0;
> @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	struct page *page = vmf->page;
>  	struct fb_info *info = vmf->vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> -	struct list_head *pos = &fbdefio->pagelist;
> +	struct fb_deferred_io_pageref *pageref;
> +	unsigned long offset;
> +	vm_fault_t ret;
> +
> +	offset = (vmf->address - vmf->vma->vm_start);
>  
>  	/* this is a callback we get when userspace first tries to
>  	write to the page. we schedule a workqueue. that workqueue
> @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>  		fbdefio->first_io(info);
>  
> +	pageref = fb_deferred_io_pageref_get(info, offset, page);
Compared to the old code we now do all the sorting and stuff without
the page locked, which seem like a big change.


> +	if (WARN_ON_ONCE(!pageref)) {
> +		ret = VM_FAULT_OOM;
> +		goto err_mutex_unlock;
> +	}
> +
>  	/*
>  	 * We want the page to remain locked from ->page_mkwrite until
>  	 * the PTE is marked dirty to avoid page_mkclean() being called
> @@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	 * Do this by locking the page here and informing the caller
>  	 * about it with VM_FAULT_LOCKED.
>  	 */
> -	lock_page(page);
> -
> -	/*
> -	 * This check is to catch the case where a new process could start
> -	 * writing to the same page through a new PTE. This new access
> -	 * can cause a call to .page_mkwrite even if the original process'
> -	 * PTE is marked writable.
> -	 *
> -	 * TODO: The lru field is owned by the page cache; hence the name.
> -	 *       We dequeue in fb_deferred_io_work() after flushing the
> -	 *       page's content into video memory. Instead of lru, fbdefio
> -	 *       should have it's own field.
> -	 */
> -	if (!list_empty(&page->lru))
> -		goto page_already_added;
> -
> -	if (unlikely(fbdefio->sort_pagelist)) {
> -		/*
> -		 * We loop through the pagelist before adding in order to
> -		 * keep the pagelist sorted. This has significant overhead
> -		 * of O(n^2) with n being the number of written pages. If
> -		 * possible, drivers should try to work with unsorted page
> -		 * lists instead.
> -		 */
> -		struct page *cur;
> -
> -		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> -			if (cur->index > page->index)
> -				break;
> -		}
> -		pos = &cur->lru;
> -	}
> -
> -	list_add_tail(&page->lru, pos);
> +	lock_page(pageref->page);
>  
> -page_already_added:
>  	mutex_unlock(&fbdefio->lock);
>  
>  	/* come back after delay to process the deferred IO */
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>  	return VM_FAULT_LOCKED;
> +
> +err_mutex_unlock:
> +	mutex_unlock(&fbdefio->lock);
> +	return ret;
>  }
>  
>  static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> @@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>  /* workqueue callback */
>  static void fb_deferred_io_work(struct work_struct *work)
>  {
> -	struct fb_info *info = container_of(work, struct fb_info,
> -						deferred_work.work);
> -	struct list_head *node, *next;
> -	struct page *cur;
> +	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
> +	struct fb_deferred_io_pageref *pageref, *next;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
>  
>  	/* here we mkclean the pages, then do all deferred IO */
>  	mutex_lock(&fbdefio->lock);
> -	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> +	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
> +		struct page *cur = pageref->page;
>  		lock_page(cur);
>  		page_mkclean(cur);
>  		unlock_page(cur);
> @@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
>  	fbdefio->deferred_io(info, &fbdefio->pagelist);
>  
>  	/* clear the list */
> -	list_for_each_safe(node, next, &fbdefio->pagelist) {
> -		list_del_init(node);
> -	}
> +	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
> +		fb_deferred_io_pageref_put(pageref, info);
> +
>  	mutex_unlock(&fbdefio->lock);
>  }
>  
> -void fb_deferred_io_init(struct fb_info *info)
> +int fb_deferred_io_init(struct fb_info *info)
>  {
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> -	struct page *page;
> -	unsigned int i;
> +	struct fb_deferred_io_pageref *pagerefs;
> +	unsigned long npagerefs, i;
> +	int ret;
>  
>  	BUG_ON(!fbdefio);
> +
> +	if (WARN_ON(!info->fix.smem_len))
> +		return -EINVAL;
> +
>  	mutex_init(&fbdefio->lock);
>  	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
>  	INIT_LIST_HEAD(&fbdefio->pagelist);
>  	if (fbdefio->delay == 0) /* set a default of 1 s */
>  		fbdefio->delay = HZ;
>  
> -	/* initialize all the page lists one time */
> -	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
> -		page = fb_deferred_io_page(info, i);
> -		INIT_LIST_HEAD(&page->lru);
> +	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
> +
> +	/* alloc a page ref for each page of the display memory */
> +	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
> +	if (!pagerefs) {
> +		ret = -ENOMEM;
> +		goto err;
>  	}
> +	for (i = 0; i < npagerefs; ++i)
> +		INIT_LIST_HEAD(&pagerefs[i].list);
> +	info->npagerefs = npagerefs;
> +	info->pagerefs = pagerefs;
> +
> +	return 0;
> +
> +err:
> +	mutex_destroy(&fbdefio->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>  
> @@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>  		page->mapping = NULL;
>  	}
>  
> +	kvfree(info->pagerefs);
>  	mutex_destroy(&fbdefio->lock);
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);

> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index f95da1af9ff6..a332590c0fae 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -201,6 +201,13 @@ struct fb_pixmap {
>  };
>  
>  #ifdef CONFIG_FB_DEFERRED_IO
> +struct fb_deferred_io_pageref {
> +	struct page *page;
> +	unsigned long offset;
> +	/* private */
> +	struct list_head list;
This is the list of pages - so this could be named pagelist...

> +};
> +
>  struct fb_deferred_io {
>  	/* delay between mkwrite and deferred handler */
>  	unsigned long delay;
> @@ -468,6 +475,8 @@ struct fb_info {
>  #endif
>  #ifdef CONFIG_FB_DEFERRED_IO
>  	struct delayed_work deferred_work;
> +	unsigned long npagerefs;
> +	struct fb_deferred_io_pageref *pagerefs;
>  	struct fb_deferred_io *fbdefio;
>  #endif
>  
> @@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
>  
>  /* drivers/video/fb_defio.c */
>  int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> -extern void fb_deferred_io_init(struct fb_info *info);
> +extern int  fb_deferred_io_init(struct fb_info *info);
>  extern void fb_deferred_io_open(struct fb_info *info,
>  				struct inode *inode,
>  				struct file *file);
> -- 
> 2.36.0

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Sam Ravnborg]

Hi Thomas.

On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
> Refactor the page-write handler for deferred I/O. Drivers use the
> function to let fbdev track written pages of mmap'ed framebuffer
> memory.

I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).

But if you consider it better keep it. Again just my thoughts when
reading the code.

	Sam

> 
> v2:
> 	* don't export the helper until we have an external caller
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a03b9c64fc61..214581ce5840 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>  
> -/* vm_ops->page_mkwrite handler */
> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +/*
> + * Adds a page to the dirty list. Requires caller to hold
> + * struct fb_deferred_io.lock. Call this from struct
> + * vm_operations_struct.page_mkwrite.
> + */
> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
> +					      struct page *page)
>  {
> -	struct page *page = vmf->page;
> -	struct fb_info *info = vmf->vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
>  	struct fb_deferred_io_pageref *pageref;
> -	unsigned long offset;
>  	vm_fault_t ret;
>  
> -	offset = (vmf->address - vmf->vma->vm_start);
> -
> -	/* this is a callback we get when userspace first tries to
> -	write to the page. we schedule a workqueue. that workqueue
> -	will eventually mkclean the touched pages and execute the
> -	deferred framebuffer IO. then if userspace touches a page
> -	again, we repeat the same scheme */
> -
> -	file_update_time(vmf->vma->vm_file);
> -
> -	/* protect against the workqueue changing the page list */
> -	mutex_lock(&fbdefio->lock);
> -
>  	/* first write in this cycle, notify the driver */
>  	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>  		fbdefio->first_io(info);
> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	 */
>  	lock_page(pageref->page);
>  
> -	mutex_unlock(&fbdefio->lock);
> -
>  	/* come back after delay to process the deferred IO */
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>  	return VM_FAULT_LOCKED;
> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	return ret;
>  }
>  
> +/*
> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
> + * @fb_info: The fbdev info structure
> + * @vmf: The VM fault
> + *
> + * This is a callback we get when userspace first tries to
> + * write to the page. We schedule a workqueue. That workqueue
> + * will eventually mkclean the touched pages and execute the
> + * deferred framebuffer IO. Then if userspace touches a page
> + * again, we repeat the same scheme.
> + *
> + * Returns:
> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
> + */
> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
> +{
> +	struct page *page = vmf->page;
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	unsigned long offset;
> +	vm_fault_t ret;
> +
> +	offset = (vmf->address - vmf->vma->vm_start);
> +
> +	file_update_time(vmf->vma->vm_file);
> +
> +	/* protect against the workqueue changing the page list */
> +	mutex_lock(&fbdefio->lock);
> +	ret = __fb_deferred_io_track_page(info, offset, page);
> +	mutex_unlock(&fbdefio->lock);
> +
> +	return ret;
> +}
> +
> +/* vm_ops->page_mkwrite handler */
> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +{
> +	struct fb_info *info = vmf->vma->vm_private_data;
> +
> +	return fb_deferred_io_page_mkwrite(info, vmf);
> +}
> +
>  static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>  	.fault		= fb_deferred_io_fault,
>  	.page_mkwrite	= fb_deferred_io_mkwrite,
> -- 
> 2.36.0

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Sam Ravnborg]

Hi Thomas.

On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
> Refactor the page-write handler for deferred I/O. Drivers use the
> function to let fbdev track written pages of mmap'ed framebuffer
> memory.

I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).

But if you consider it better keep it. Again just my thoughts when
reading the code.

	Sam

> 
> v2:
> 	* don't export the helper until we have an external caller
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a03b9c64fc61..214581ce5840 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>  
> -/* vm_ops->page_mkwrite handler */
> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +/*
> + * Adds a page to the dirty list. Requires caller to hold
> + * struct fb_deferred_io.lock. Call this from struct
> + * vm_operations_struct.page_mkwrite.
> + */
> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
> +					      struct page *page)
>  {
> -	struct page *page = vmf->page;
> -	struct fb_info *info = vmf->vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
>  	struct fb_deferred_io_pageref *pageref;
> -	unsigned long offset;
>  	vm_fault_t ret;
>  
> -	offset = (vmf->address - vmf->vma->vm_start);
> -
> -	/* this is a callback we get when userspace first tries to
> -	write to the page. we schedule a workqueue. that workqueue
> -	will eventually mkclean the touched pages and execute the
> -	deferred framebuffer IO. then if userspace touches a page
> -	again, we repeat the same scheme */
> -
> -	file_update_time(vmf->vma->vm_file);
> -
> -	/* protect against the workqueue changing the page list */
> -	mutex_lock(&fbdefio->lock);
> -
>  	/* first write in this cycle, notify the driver */
>  	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>  		fbdefio->first_io(info);
> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	 */
>  	lock_page(pageref->page);
>  
> -	mutex_unlock(&fbdefio->lock);
> -
>  	/* come back after delay to process the deferred IO */
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>  	return VM_FAULT_LOCKED;
> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	return ret;
>  }
>  
> +/*
> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
> + * @fb_info: The fbdev info structure
> + * @vmf: The VM fault
> + *
> + * This is a callback we get when userspace first tries to
> + * write to the page. We schedule a workqueue. That workqueue
> + * will eventually mkclean the touched pages and execute the
> + * deferred framebuffer IO. Then if userspace touches a page
> + * again, we repeat the same scheme.
> + *
> + * Returns:
> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
> + */
> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
> +{
> +	struct page *page = vmf->page;
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	unsigned long offset;
> +	vm_fault_t ret;
> +
> +	offset = (vmf->address - vmf->vma->vm_start);
> +
> +	file_update_time(vmf->vma->vm_file);
> +
> +	/* protect against the workqueue changing the page list */
> +	mutex_lock(&fbdefio->lock);
> +	ret = __fb_deferred_io_track_page(info, offset, page);
> +	mutex_unlock(&fbdefio->lock);
> +
> +	return ret;
> +}
> +
> +/* vm_ops->page_mkwrite handler */
> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +{
> +	struct fb_info *info = vmf->vma->vm_private_data;
> +
> +	return fb_deferred_io_page_mkwrite(info, vmf);
> +}
> +
>  static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>  	.fault		= fb_deferred_io_fault,
>  	.page_mkwrite	= fb_deferred_io_mkwrite,
> -- 
> 2.36.0

Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3690 bytes --]

Hi Sam

Am 25.04.22 um 19:46 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Apr 25, 2022 at 07:26:32PM +0200, Sam Ravnborg wrote:
>> Hi Thomas,
>>
>>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>>> index 6aaf6d0abf39..6924d489a289 100644
>>> --- a/drivers/video/fbdev/core/fb_defio.c
>>> +++ b/drivers/video/fbdev/core/fb_defio.c
>>> @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>>>   	vma->vm_private_data = info;
>>>   	return 0;
>>>   }
>>> +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>>>   
>>>   /* workqueue callback */
>>>   static void fb_deferred_io_work(struct work_struct *work)
>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>> index 84427470367b..52440e3f8f69 100644
>>> --- a/drivers/video/fbdev/core/fbmem.c
>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>> @@ -1334,7 +1334,6 @@ static int
>>>   fb_mmap(struct file *file, struct vm_area_struct * vma)
>>>   {
>>>   	struct fb_info *info = file_fb_info(file);
>>> -	int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
>>>   	unsigned long mmio_pgoff;
>>>   	unsigned long start;
>>>   	u32 len;
>>> @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>>>   		return -ENODEV;
>>>   	mutex_lock(&info->mm_lock);
>>>   
>>> -	fb_mmap_fn = info->fbops->fb_mmap;
>>> -
>>> -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
>>> -	if (info->fbdefio)
>>> -		fb_mmap_fn = fb_deferred_io_mmap;
>>> -#endif
>>> -
>>> -	if (fb_mmap_fn) {
>>> +	if (info->fbops->fb_mmap) {
>>>   		int res;
>>>   
>>>   		/*
>>> @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
>>>   		 * SME protection is removed ahead of the call
>>>   		 */
>>>   		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>>> -		res = fb_mmap_fn(info, vma);
>>> +		res = info->fbops->fb_mmap(info, vma);
>>>   		mutex_unlock(&info->mm_lock);
>>>   		return res;
>>>   	}
>>>   
>>> +	/*
>>> +	 * FB deferred I/O wants you to handle mmap in your drivers. At a
>>> +	 * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
>>> +	 */
>>> +	if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for defered I/O.\n"))
>>> +		return -ENODEV;
>>> +
>>
>> If not configured - then why not just call fb_deferred_io_mmap(), as
>> this seems to be the default implementation anyway.
>> Then drivers that needs it can override - and the rest fallback to the
>> default.
> 
> Just to be clear - I already read:
> "
> Leave the mmap handling to drivers and expect them to call the
> helper for deferred I/O by thmeselves.
> "
> 
> But this does not help me understand why we need to explicit do what
> could be a simple default implementation.
> Chances are that I am stupid and it is obvious..

That's no stupid question. I didn't want to use a default implementation 
because there's no single one that severs all purposes. The current code 
all uses the same helper, but this will change with later patchsets. At 
some point, GEM is supposed to implement some of the logic for deferred 
I/O and will have to set it's own helpers. This can be seen in patches 8 
and 9 of [1]. I think it's better to clearly fail here than to provide a 
default implementation.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/20220303205839.28484-1-tzimmermann@suse.de/


> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 14268 bytes --]

Hi

Am 25.04.22 um 20:17 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> a little ramblings below. Just my thoughts while trying to understand
> the code - especially since I looked at it before.

Thanks for reviewing the patches.

> 
> 	Sam
> 
> On Mon, Apr 25, 2022 at 01:27:50PM +0200, Thomas Zimmermann wrote:
>> Store the per-page state for fbdev's deferred I/O in struct
>> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
>> that have to be written back to video memory. Update all affected
>> drivers.
>>
>> As with pages before, fbdev acquires a pageref when an mmaped page
>> of the framebuffer is being written to. It holds the pageref in a
>> list of all currently written pagerefs until it flushes the written
>> pages to video memory. Writeback occurs periodically. After writeback
>> fbdev releases all pagerefs and builds up a new dirty list until the
>> next writeback occurs.
>>
>> Using pagerefs has a number of benefits.
>>
>> For pages of the framebuffer, the deferred I/O code used struct
>> page.lru as an entry into the list of dirty pages. The lru field is
>> owned by the page cache, which makes deferred I/O incompatible with
>> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
>> struct fb_deferred_io_pageref now provides an entry into a list of
>> dirty framebuffer pages, freeing lru for use with the page cache.
>>
>> Drivers also assumed that struct page.index is the page offset into
>> the framebuffer. This is not true for DRM buffers, which are located
>> at various offset within a mapped area. struct fb_deferred_io_pageref
>> explicitly stores an offset into the framebuffer. struct page.index
>> is now only the page offset into the mapped area.
>>
>> These changes will allow DRM to use fbdev deferred I/O without an
>> intermediate shadow buffer.
>>
>> v2:
>> 	* minor fixes in commit message
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
> 
> 
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> index 02a4a4fa3da3..db02e17e12b6 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
>>   	struct vmw_fb_par *par = info->par;
>>   	unsigned long start, end, min, max;
>>   	unsigned long flags;
>> -	struct page *page;
>> +	struct fb_deferred_io_pageref *pageref;
>>   	int y1, y2;
>>   
>>   	min = ULONG_MAX;
>>   	max = 0;
>> -	list_for_each_entry(page, pagelist, lru) {
>> +	list_for_each_entry(pageref, pagelist, list) {
>> +		struct page *page = pageref->page;
>>   		start = page->index << PAGE_SHIFT;
>>   		end = start + PAGE_SIZE - 1;
>>   		min = min(min, start);
> 
> This is the same change in all deferred_io drivers like above.
> We now traverse pageref->list where pagelist points to the head.
> 
> It took me a while to understand that pagelist points to a list of
> fb_deferred_io_pageref and not a list of pages.
> I had been helped, had this been renamed to s/pagelist/pagereflist/.
> 
> If you see no point in this then just ignore my comment, I have not
> stared at kernel code for a while and is thus easy to confuse.

It's a good point. There's already a lot going on in this patch and I 
wanted to minimize the fallout. But I'll add an additional patch that 
fixes the naming.

> 
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index 6924d489a289..a03b9c64fc61 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
>>   	return page;
>>   }
>>   
>> +static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
>> +								 unsigned long offset,
>> +								 struct page *page)
>> +{
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	struct list_head *pos = &fbdefio->pagelist;
>> +	unsigned long pgoff = offset >> PAGE_SHIFT;
>> +	struct fb_deferred_io_pageref *pageref, *cur;
>> +
>> +	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
>> +		return NULL; /* incorrect allocation size */
>> +
>> +	/* 1:1 mapping between pageref and page offset */
>> +	pageref = &info->pagerefs[pgoff];
>> +
>> +	/*
>> +	 * This check is to catch the case where a new process could start
>> +	 * writing to the same page through a new PTE. This new access
>> +	 * can cause a call to .page_mkwrite even if the original process'
>> +	 * PTE is marked writable.
>> +	 */
>> +	if (!list_empty(&pageref->list))
>> +		goto pageref_already_added;
>> +
>> +	pageref->page = page;
>> +	pageref->offset = pgoff << PAGE_SHIFT;
>> +
>> +	if (unlikely(fbdefio->sort_pagelist)) {
>> +		/*
>> +		 * We loop through the list of pagerefs before adding, in
>> +		 * order to keep the pagerefs sorted. This has significant
>> +		 * overhead of O(n^2) with n being the number of written
>> +		 * pages. If possible, drivers should try to work with
>> +		 * unsorted page lists instead.
>> +		 */
>> +		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
>> +			if (cur > pageref)
>> +				break;
>> +		}
>> +		pos = &cur->list;
>> +	}
>> +
>> +	list_add_tail(&pageref->list, pos);
>> +
>> +pageref_already_added:
>> +	return pageref;
>> +}
>> +
>> +static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
>> +				       struct fb_info *info)
>> +{
>> +	list_del_init(&pageref->list);
>> +}
>> +
>>   /* this is to find and return the vmalloc-ed fb pages */
>>   static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>>   {
>> @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>>   		printk(KERN_ERR "no mapping available\n");
>>   
>>   	BUG_ON(!page->mapping);
>> -	page->index = vmf->pgoff;
>> +	page->index = vmf->pgoff; /* for page_mkclean() */
>>   
>>   	vmf->page = page;
>>   	return 0;
>> @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	struct page *page = vmf->page;
>>   	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>> -	struct list_head *pos = &fbdefio->pagelist;
>> +	struct fb_deferred_io_pageref *pageref;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>>   
>>   	/* this is a callback we get when userspace first tries to
>>   	write to the page. we schedule a workqueue. that workqueue
>> @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>>   
>> +	pageref = fb_deferred_io_pageref_get(info, offset, page);
> Compared to the old code we now do all the sorting and stuff without
> the page locked, which seem like a big change.

We never touch any of the page's fields in fb_deferred_io_pageref_get(). 
It's only used to initialize the pageref's page pointer. The pagerefs 
are all protected by fbdev-internal locking.  Is there a reason why we 
should further hold the page lock?

All sorting is done by the pageref addresses, which implicitly 
correspond to 'offset'. After looking at the new function again, I'll 
change it to sort directly by offset. It's clearer in its intend.

> 
> 
>> +	if (WARN_ON_ONCE(!pageref)) {
>> +		ret = VM_FAULT_OOM;
>> +		goto err_mutex_unlock;
>> +	}
>> +
>>   	/*
>>   	 * We want the page to remain locked from ->page_mkwrite until
>>   	 * the PTE is marked dirty to avoid page_mkclean() being called
>> @@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 * Do this by locking the page here and informing the caller
>>   	 * about it with VM_FAULT_LOCKED.
>>   	 */
>> -	lock_page(page);
>> -
>> -	/*
>> -	 * This check is to catch the case where a new process could start
>> -	 * writing to the same page through a new PTE. This new access
>> -	 * can cause a call to .page_mkwrite even if the original process'
>> -	 * PTE is marked writable.
>> -	 *
>> -	 * TODO: The lru field is owned by the page cache; hence the name.
>> -	 *       We dequeue in fb_deferred_io_work() after flushing the
>> -	 *       page's content into video memory. Instead of lru, fbdefio
>> -	 *       should have it's own field.
>> -	 */
>> -	if (!list_empty(&page->lru))
>> -		goto page_already_added;
>> -
>> -	if (unlikely(fbdefio->sort_pagelist)) {
>> -		/*
>> -		 * We loop through the pagelist before adding in order to
>> -		 * keep the pagelist sorted. This has significant overhead
>> -		 * of O(n^2) with n being the number of written pages. If
>> -		 * possible, drivers should try to work with unsorted page
>> -		 * lists instead.
>> -		 */
>> -		struct page *cur;
>> -
>> -		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>> -			if (cur->index > page->index)
>> -				break;
>> -		}
>> -		pos = &cur->lru;
>> -	}
>> -
>> -	list_add_tail(&page->lru, pos);
>> +	lock_page(pageref->page);

The page lock is taken here.

Best regards
Thomas

>>   
>> -page_already_added:
>>   	mutex_unlock(&fbdefio->lock);
>>   
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> +
>> +err_mutex_unlock:
>> +	mutex_unlock(&fbdefio->lock);
>> +	return ret;
>>   }
>>   
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>> @@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>>   /* workqueue callback */
>>   static void fb_deferred_io_work(struct work_struct *work)
>>   {
>> -	struct fb_info *info = container_of(work, struct fb_info,
>> -						deferred_work.work);
>> -	struct list_head *node, *next;
>> -	struct page *cur;
>> +	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
>> +	struct fb_deferred_io_pageref *pageref, *next;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   
>>   	/* here we mkclean the pages, then do all deferred IO */
>>   	mutex_lock(&fbdefio->lock);
>> -	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>> +	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
>> +		struct page *cur = pageref->page;
>>   		lock_page(cur);
>>   		page_mkclean(cur);
>>   		unlock_page(cur);
>> @@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
>>   	fbdefio->deferred_io(info, &fbdefio->pagelist);
>>   
>>   	/* clear the list */
>> -	list_for_each_safe(node, next, &fbdefio->pagelist) {
>> -		list_del_init(node);
>> -	}
>> +	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
>> +		fb_deferred_io_pageref_put(pageref, info);
>> +
>>   	mutex_unlock(&fbdefio->lock);
>>   }
>>   
>> -void fb_deferred_io_init(struct fb_info *info)
>> +int fb_deferred_io_init(struct fb_info *info)
>>   {
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>> -	struct page *page;
>> -	unsigned int i;
>> +	struct fb_deferred_io_pageref *pagerefs;
>> +	unsigned long npagerefs, i;
>> +	int ret;
>>   
>>   	BUG_ON(!fbdefio);
>> +
>> +	if (WARN_ON(!info->fix.smem_len))
>> +		return -EINVAL;
>> +
>>   	mutex_init(&fbdefio->lock);
>>   	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
>>   	INIT_LIST_HEAD(&fbdefio->pagelist);
>>   	if (fbdefio->delay == 0) /* set a default of 1 s */
>>   		fbdefio->delay = HZ;
>>   
>> -	/* initialize all the page lists one time */
>> -	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
>> -		page = fb_deferred_io_page(info, i);
>> -		INIT_LIST_HEAD(&page->lru);
>> +	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
>> +
>> +	/* alloc a page ref for each page of the display memory */
>> +	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
>> +	if (!pagerefs) {
>> +		ret = -ENOMEM;
>> +		goto err;
>>   	}
>> +	for (i = 0; i < npagerefs; ++i)
>> +		INIT_LIST_HEAD(&pagerefs[i].list);
>> +	info->npagerefs = npagerefs;
>> +	info->pagerefs = pagerefs;
>> +
>> +	return 0;
>> +
>> +err:
>> +	mutex_destroy(&fbdefio->lock);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>>   
>> @@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>>   		page->mapping = NULL;
>>   	}
>>   
>> +	kvfree(info->pagerefs);
>>   	mutex_destroy(&fbdefio->lock);
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> 
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index f95da1af9ff6..a332590c0fae 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -201,6 +201,13 @@ struct fb_pixmap {
>>   };
>>   
>>   #ifdef CONFIG_FB_DEFERRED_IO
>> +struct fb_deferred_io_pageref {
>> +	struct page *page;
>> +	unsigned long offset;
>> +	/* private */
>> +	struct list_head list;
> This is the list of pages - so this could be named pagelist...
> 
>> +};
>> +
>>   struct fb_deferred_io {
>>   	/* delay between mkwrite and deferred handler */
>>   	unsigned long delay;
>> @@ -468,6 +475,8 @@ struct fb_info {
>>   #endif
>>   #ifdef CONFIG_FB_DEFERRED_IO
>>   	struct delayed_work deferred_work;
>> +	unsigned long npagerefs;
>> +	struct fb_deferred_io_pageref *pagerefs;
>>   	struct fb_deferred_io *fbdefio;
>>   #endif
>>   
>> @@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
>>   
>>   /* drivers/video/fb_defio.c */
>>   int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
>> -extern void fb_deferred_io_init(struct fb_info *info);
>> +extern int  fb_deferred_io_init(struct fb_info *info);
>>   extern void fb_deferred_io_open(struct fb_info *info,
>>   				struct inode *inode,
>>   				struct file *file);
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 14268 bytes --]

Hi

Am 25.04.22 um 20:17 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> a little ramblings below. Just my thoughts while trying to understand
> the code - especially since I looked at it before.

Thanks for reviewing the patches.

> 
> 	Sam
> 
> On Mon, Apr 25, 2022 at 01:27:50PM +0200, Thomas Zimmermann wrote:
>> Store the per-page state for fbdev's deferred I/O in struct
>> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
>> that have to be written back to video memory. Update all affected
>> drivers.
>>
>> As with pages before, fbdev acquires a pageref when an mmaped page
>> of the framebuffer is being written to. It holds the pageref in a
>> list of all currently written pagerefs until it flushes the written
>> pages to video memory. Writeback occurs periodically. After writeback
>> fbdev releases all pagerefs and builds up a new dirty list until the
>> next writeback occurs.
>>
>> Using pagerefs has a number of benefits.
>>
>> For pages of the framebuffer, the deferred I/O code used struct
>> page.lru as an entry into the list of dirty pages. The lru field is
>> owned by the page cache, which makes deferred I/O incompatible with
>> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
>> struct fb_deferred_io_pageref now provides an entry into a list of
>> dirty framebuffer pages, freeing lru for use with the page cache.
>>
>> Drivers also assumed that struct page.index is the page offset into
>> the framebuffer. This is not true for DRM buffers, which are located
>> at various offset within a mapped area. struct fb_deferred_io_pageref
>> explicitly stores an offset into the framebuffer. struct page.index
>> is now only the page offset into the mapped area.
>>
>> These changes will allow DRM to use fbdev deferred I/O without an
>> intermediate shadow buffer.
>>
>> v2:
>> 	* minor fixes in commit message
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
> 
> 
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> index 02a4a4fa3da3..db02e17e12b6 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
>>   	struct vmw_fb_par *par = info->par;
>>   	unsigned long start, end, min, max;
>>   	unsigned long flags;
>> -	struct page *page;
>> +	struct fb_deferred_io_pageref *pageref;
>>   	int y1, y2;
>>   
>>   	min = ULONG_MAX;
>>   	max = 0;
>> -	list_for_each_entry(page, pagelist, lru) {
>> +	list_for_each_entry(pageref, pagelist, list) {
>> +		struct page *page = pageref->page;
>>   		start = page->index << PAGE_SHIFT;
>>   		end = start + PAGE_SIZE - 1;
>>   		min = min(min, start);
> 
> This is the same change in all deferred_io drivers like above.
> We now traverse pageref->list where pagelist points to the head.
> 
> It took me a while to understand that pagelist points to a list of
> fb_deferred_io_pageref and not a list of pages.
> I had been helped, had this been renamed to s/pagelist/pagereflist/.
> 
> If you see no point in this then just ignore my comment, I have not
> stared at kernel code for a while and is thus easy to confuse.

It's a good point. There's already a lot going on in this patch and I 
wanted to minimize the fallout. But I'll add an additional patch that 
fixes the naming.

> 
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index 6924d489a289..a03b9c64fc61 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs
>>   	return page;
>>   }
>>   
>> +static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info *info,
>> +								 unsigned long offset,
>> +								 struct page *page)
>> +{
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	struct list_head *pos = &fbdefio->pagelist;
>> +	unsigned long pgoff = offset >> PAGE_SHIFT;
>> +	struct fb_deferred_io_pageref *pageref, *cur;
>> +
>> +	if (WARN_ON_ONCE(pgoff >= info->npagerefs))
>> +		return NULL; /* incorrect allocation size */
>> +
>> +	/* 1:1 mapping between pageref and page offset */
>> +	pageref = &info->pagerefs[pgoff];
>> +
>> +	/*
>> +	 * This check is to catch the case where a new process could start
>> +	 * writing to the same page through a new PTE. This new access
>> +	 * can cause a call to .page_mkwrite even if the original process'
>> +	 * PTE is marked writable.
>> +	 */
>> +	if (!list_empty(&pageref->list))
>> +		goto pageref_already_added;
>> +
>> +	pageref->page = page;
>> +	pageref->offset = pgoff << PAGE_SHIFT;
>> +
>> +	if (unlikely(fbdefio->sort_pagelist)) {
>> +		/*
>> +		 * We loop through the list of pagerefs before adding, in
>> +		 * order to keep the pagerefs sorted. This has significant
>> +		 * overhead of O(n^2) with n being the number of written
>> +		 * pages. If possible, drivers should try to work with
>> +		 * unsorted page lists instead.
>> +		 */
>> +		list_for_each_entry(cur, &info->fbdefio->pagelist, list) {
>> +			if (cur > pageref)
>> +				break;
>> +		}
>> +		pos = &cur->list;
>> +	}
>> +
>> +	list_add_tail(&pageref->list, pos);
>> +
>> +pageref_already_added:
>> +	return pageref;
>> +}
>> +
>> +static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
>> +				       struct fb_info *info)
>> +{
>> +	list_del_init(&pageref->list);
>> +}
>> +
>>   /* this is to find and return the vmalloc-ed fb pages */
>>   static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>>   {
>> @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>>   		printk(KERN_ERR "no mapping available\n");
>>   
>>   	BUG_ON(!page->mapping);
>> -	page->index = vmf->pgoff;
>> +	page->index = vmf->pgoff; /* for page_mkclean() */
>>   
>>   	vmf->page = page;
>>   	return 0;
>> @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	struct page *page = vmf->page;
>>   	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>> -	struct list_head *pos = &fbdefio->pagelist;
>> +	struct fb_deferred_io_pageref *pageref;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>>   
>>   	/* this is a callback we get when userspace first tries to
>>   	write to the page. we schedule a workqueue. that workqueue
>> @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>>   
>> +	pageref = fb_deferred_io_pageref_get(info, offset, page);
> Compared to the old code we now do all the sorting and stuff without
> the page locked, which seem like a big change.

We never touch any of the page's fields in fb_deferred_io_pageref_get(). 
It's only used to initialize the pageref's page pointer. The pagerefs 
are all protected by fbdev-internal locking.  Is there a reason why we 
should further hold the page lock?

All sorting is done by the pageref addresses, which implicitly 
correspond to 'offset'. After looking at the new function again, I'll 
change it to sort directly by offset. It's clearer in its intend.

> 
> 
>> +	if (WARN_ON_ONCE(!pageref)) {
>> +		ret = VM_FAULT_OOM;
>> +		goto err_mutex_unlock;
>> +	}
>> +
>>   	/*
>>   	 * We want the page to remain locked from ->page_mkwrite until
>>   	 * the PTE is marked dirty to avoid page_mkclean() being called
>> @@ -120,47 +184,17 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 * Do this by locking the page here and informing the caller
>>   	 * about it with VM_FAULT_LOCKED.
>>   	 */
>> -	lock_page(page);
>> -
>> -	/*
>> -	 * This check is to catch the case where a new process could start
>> -	 * writing to the same page through a new PTE. This new access
>> -	 * can cause a call to .page_mkwrite even if the original process'
>> -	 * PTE is marked writable.
>> -	 *
>> -	 * TODO: The lru field is owned by the page cache; hence the name.
>> -	 *       We dequeue in fb_deferred_io_work() after flushing the
>> -	 *       page's content into video memory. Instead of lru, fbdefio
>> -	 *       should have it's own field.
>> -	 */
>> -	if (!list_empty(&page->lru))
>> -		goto page_already_added;
>> -
>> -	if (unlikely(fbdefio->sort_pagelist)) {
>> -		/*
>> -		 * We loop through the pagelist before adding in order to
>> -		 * keep the pagelist sorted. This has significant overhead
>> -		 * of O(n^2) with n being the number of written pages. If
>> -		 * possible, drivers should try to work with unsorted page
>> -		 * lists instead.
>> -		 */
>> -		struct page *cur;
>> -
>> -		list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>> -			if (cur->index > page->index)
>> -				break;
>> -		}
>> -		pos = &cur->lru;
>> -	}
>> -
>> -	list_add_tail(&page->lru, pos);
>> +	lock_page(pageref->page);

The page lock is taken here.

Best regards
Thomas

>>   
>> -page_already_added:
>>   	mutex_unlock(&fbdefio->lock);
>>   
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> +
>> +err_mutex_unlock:
>> +	mutex_unlock(&fbdefio->lock);
>> +	return ret;
>>   }
>>   
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>> @@ -186,15 +220,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>>   /* workqueue callback */
>>   static void fb_deferred_io_work(struct work_struct *work)
>>   {
>> -	struct fb_info *info = container_of(work, struct fb_info,
>> -						deferred_work.work);
>> -	struct list_head *node, *next;
>> -	struct page *cur;
>> +	struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
>> +	struct fb_deferred_io_pageref *pageref, *next;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   
>>   	/* here we mkclean the pages, then do all deferred IO */
>>   	mutex_lock(&fbdefio->lock);
>> -	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>> +	list_for_each_entry(pageref, &fbdefio->pagelist, list) {
>> +		struct page *cur = pageref->page;
>>   		lock_page(cur);
>>   		page_mkclean(cur);
>>   		unlock_page(cur);
>> @@ -204,30 +237,48 @@ static void fb_deferred_io_work(struct work_struct *work)
>>   	fbdefio->deferred_io(info, &fbdefio->pagelist);
>>   
>>   	/* clear the list */
>> -	list_for_each_safe(node, next, &fbdefio->pagelist) {
>> -		list_del_init(node);
>> -	}
>> +	list_for_each_entry_safe(pageref, next, &fbdefio->pagelist, list)
>> +		fb_deferred_io_pageref_put(pageref, info);
>> +
>>   	mutex_unlock(&fbdefio->lock);
>>   }
>>   
>> -void fb_deferred_io_init(struct fb_info *info)
>> +int fb_deferred_io_init(struct fb_info *info)
>>   {
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>> -	struct page *page;
>> -	unsigned int i;
>> +	struct fb_deferred_io_pageref *pagerefs;
>> +	unsigned long npagerefs, i;
>> +	int ret;
>>   
>>   	BUG_ON(!fbdefio);
>> +
>> +	if (WARN_ON(!info->fix.smem_len))
>> +		return -EINVAL;
>> +
>>   	mutex_init(&fbdefio->lock);
>>   	INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
>>   	INIT_LIST_HEAD(&fbdefio->pagelist);
>>   	if (fbdefio->delay == 0) /* set a default of 1 s */
>>   		fbdefio->delay = HZ;
>>   
>> -	/* initialize all the page lists one time */
>> -	for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
>> -		page = fb_deferred_io_page(info, i);
>> -		INIT_LIST_HEAD(&page->lru);
>> +	npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
>> +
>> +	/* alloc a page ref for each page of the display memory */
>> +	pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
>> +	if (!pagerefs) {
>> +		ret = -ENOMEM;
>> +		goto err;
>>   	}
>> +	for (i = 0; i < npagerefs; ++i)
>> +		INIT_LIST_HEAD(&pagerefs[i].list);
>> +	info->npagerefs = npagerefs;
>> +	info->pagerefs = pagerefs;
>> +
>> +	return 0;
>> +
>> +err:
>> +	mutex_destroy(&fbdefio->lock);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>>   
>> @@ -254,6 +305,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>>   		page->mapping = NULL;
>>   	}
>>   
>> +	kvfree(info->pagerefs);
>>   	mutex_destroy(&fbdefio->lock);
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> 
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index f95da1af9ff6..a332590c0fae 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -201,6 +201,13 @@ struct fb_pixmap {
>>   };
>>   
>>   #ifdef CONFIG_FB_DEFERRED_IO
>> +struct fb_deferred_io_pageref {
>> +	struct page *page;
>> +	unsigned long offset;
>> +	/* private */
>> +	struct list_head list;
> This is the list of pages - so this could be named pagelist...
> 
>> +};
>> +
>>   struct fb_deferred_io {
>>   	/* delay between mkwrite and deferred handler */
>>   	unsigned long delay;
>> @@ -468,6 +475,8 @@ struct fb_info {
>>   #endif
>>   #ifdef CONFIG_FB_DEFERRED_IO
>>   	struct delayed_work deferred_work;
>> +	unsigned long npagerefs;
>> +	struct fb_deferred_io_pageref *pagerefs;
>>   	struct fb_deferred_io *fbdefio;
>>   #endif
>>   
>> @@ -661,7 +670,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
>>   
>>   /* drivers/video/fb_defio.c */
>>   int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
>> -extern void fb_deferred_io_init(struct fb_info *info);
>> +extern int  fb_deferred_io_init(struct fb_info *info);
>>   extern void fb_deferred_io_open(struct fb_info *info,
>>   				struct inode *inode,
>>   				struct file *file);
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5404 bytes --]

Hi

Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
>> Refactor the page-write handler for deferred I/O. Drivers use the
>> function to let fbdev track written pages of mmap'ed framebuffer
>> memory.
> 
> I like how the comments got a brush up and a little more info was added.
> But I do not see the point of the refactoring - the code is equally
> readable before and after - maybe even easier before (modulus the
> improved comments).
> 
> But if you consider it better keep it. Again just my thoughts when
> reading the code.

The comes from patch of the larger GEM refactoring patches. [1] 
fb_deferred_io_page_mkwrite() is later supposed to be exported for use 
with DRM.

The patch isn't strictly necessary, but it doesn't do any harm either. I 
added it to the patchset, so that I don't have to deal with potential 
bit rot later on.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/20220303205839.28484-5-tzimmermann@suse.de/

> 
> 	Sam
> 
>>
>> v2:
>> 	* don't export the helper until we have an external caller
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index a03b9c64fc61..214581ce5840 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>>   
>> -/* vm_ops->page_mkwrite handler */
>> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +/*
>> + * Adds a page to the dirty list. Requires caller to hold
>> + * struct fb_deferred_io.lock. Call this from struct
>> + * vm_operations_struct.page_mkwrite.
>> + */
>> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
>> +					      struct page *page)
>>   {
>> -	struct page *page = vmf->page;
>> -	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   	struct fb_deferred_io_pageref *pageref;
>> -	unsigned long offset;
>>   	vm_fault_t ret;
>>   
>> -	offset = (vmf->address - vmf->vma->vm_start);
>> -
>> -	/* this is a callback we get when userspace first tries to
>> -	write to the page. we schedule a workqueue. that workqueue
>> -	will eventually mkclean the touched pages and execute the
>> -	deferred framebuffer IO. then if userspace touches a page
>> -	again, we repeat the same scheme */
>> -
>> -	file_update_time(vmf->vma->vm_file);
>> -
>> -	/* protect against the workqueue changing the page list */
>> -	mutex_lock(&fbdefio->lock);
>> -
>>   	/* first write in this cycle, notify the driver */
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 */
>>   	lock_page(pageref->page);
>>   
>> -	mutex_unlock(&fbdefio->lock);
>> -
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	return ret;
>>   }
>>   
>> +/*
>> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
>> + * @fb_info: The fbdev info structure
>> + * @vmf: The VM fault
>> + *
>> + * This is a callback we get when userspace first tries to
>> + * write to the page. We schedule a workqueue. That workqueue
>> + * will eventually mkclean the touched pages and execute the
>> + * deferred framebuffer IO. Then if userspace touches a page
>> + * again, we repeat the same scheme.
>> + *
>> + * Returns:
>> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
>> + */
>> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
>> +{
>> +	struct page *page = vmf->page;
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>> +
>> +	file_update_time(vmf->vma->vm_file);
>> +
>> +	/* protect against the workqueue changing the page list */
>> +	mutex_lock(&fbdefio->lock);
>> +	ret = __fb_deferred_io_track_page(info, offset, page);
>> +	mutex_unlock(&fbdefio->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/* vm_ops->page_mkwrite handler */
>> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +{
>> +	struct fb_info *info = vmf->vma->vm_private_data;
>> +
>> +	return fb_deferred_io_page_mkwrite(info, vmf);
>> +}
>> +
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>>   	.fault		= fb_deferred_io_fault,
>>   	.page_mkwrite	= fb_deferred_io_mkwrite,
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5404 bytes --]

Hi

Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
>> Refactor the page-write handler for deferred I/O. Drivers use the
>> function to let fbdev track written pages of mmap'ed framebuffer
>> memory.
> 
> I like how the comments got a brush up and a little more info was added.
> But I do not see the point of the refactoring - the code is equally
> readable before and after - maybe even easier before (modulus the
> improved comments).
> 
> But if you consider it better keep it. Again just my thoughts when
> reading the code.

The comes from patch of the larger GEM refactoring patches. [1] 
fb_deferred_io_page_mkwrite() is later supposed to be exported for use 
with DRM.

The patch isn't strictly necessary, but it doesn't do any harm either. I 
added it to the patchset, so that I don't have to deal with potential 
bit rot later on.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/20220303205839.28484-5-tzimmermann@suse.de/

> 
> 	Sam
> 
>>
>> v2:
>> 	* don't export the helper until we have an external caller
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index a03b9c64fc61..214581ce5840 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>>   
>> -/* vm_ops->page_mkwrite handler */
>> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +/*
>> + * Adds a page to the dirty list. Requires caller to hold
>> + * struct fb_deferred_io.lock. Call this from struct
>> + * vm_operations_struct.page_mkwrite.
>> + */
>> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
>> +					      struct page *page)
>>   {
>> -	struct page *page = vmf->page;
>> -	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   	struct fb_deferred_io_pageref *pageref;
>> -	unsigned long offset;
>>   	vm_fault_t ret;
>>   
>> -	offset = (vmf->address - vmf->vma->vm_start);
>> -
>> -	/* this is a callback we get when userspace first tries to
>> -	write to the page. we schedule a workqueue. that workqueue
>> -	will eventually mkclean the touched pages and execute the
>> -	deferred framebuffer IO. then if userspace touches a page
>> -	again, we repeat the same scheme */
>> -
>> -	file_update_time(vmf->vma->vm_file);
>> -
>> -	/* protect against the workqueue changing the page list */
>> -	mutex_lock(&fbdefio->lock);
>> -
>>   	/* first write in this cycle, notify the driver */
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 */
>>   	lock_page(pageref->page);
>>   
>> -	mutex_unlock(&fbdefio->lock);
>> -
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	return ret;
>>   }
>>   
>> +/*
>> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
>> + * @fb_info: The fbdev info structure
>> + * @vmf: The VM fault
>> + *
>> + * This is a callback we get when userspace first tries to
>> + * write to the page. We schedule a workqueue. That workqueue
>> + * will eventually mkclean the touched pages and execute the
>> + * deferred framebuffer IO. Then if userspace touches a page
>> + * again, we repeat the same scheme.
>> + *
>> + * Returns:
>> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
>> + */
>> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
>> +{
>> +	struct page *page = vmf->page;
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>> +
>> +	file_update_time(vmf->vma->vm_file);
>> +
>> +	/* protect against the workqueue changing the page list */
>> +	mutex_lock(&fbdefio->lock);
>> +	ret = __fb_deferred_io_track_page(info, offset, page);
>> +	mutex_unlock(&fbdefio->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/* vm_ops->page_mkwrite handler */
>> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +{
>> +	struct fb_info *info = vmf->vma->vm_private_data;
>> +
>> +	return fb_deferred_io_page_mkwrite(info, vmf);
>> +}
>> +
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>>   	.fault		= fb_deferred_io_fault,
>>   	.page_mkwrite	= fb_deferred_io_mkwrite,
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5100 bytes --]

Hi

Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
>> Refactor the page-write handler for deferred I/O. Drivers use the
>> function to let fbdev track written pages of mmap'ed framebuffer
>> memory.
> 
> I like how the comments got a brush up and a little more info was added.
> But I do not see the point of the refactoring - the code is equally
> readable before and after - maybe even easier before (modulus the
> improved comments).

FYI I'm going to move the locking into the track-page helper, which 
makes the code more readable.

Best regards
Thomas

> 
> But if you consider it better keep it. Again just my thoughts when
> reading the code.
> 
> 	Sam
> 
>>
>> v2:
>> 	* don't export the helper until we have an external caller
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index a03b9c64fc61..214581ce5840 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>>   
>> -/* vm_ops->page_mkwrite handler */
>> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +/*
>> + * Adds a page to the dirty list. Requires caller to hold
>> + * struct fb_deferred_io.lock. Call this from struct
>> + * vm_operations_struct.page_mkwrite.
>> + */
>> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
>> +					      struct page *page)
>>   {
>> -	struct page *page = vmf->page;
>> -	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   	struct fb_deferred_io_pageref *pageref;
>> -	unsigned long offset;
>>   	vm_fault_t ret;
>>   
>> -	offset = (vmf->address - vmf->vma->vm_start);
>> -
>> -	/* this is a callback we get when userspace first tries to
>> -	write to the page. we schedule a workqueue. that workqueue
>> -	will eventually mkclean the touched pages and execute the
>> -	deferred framebuffer IO. then if userspace touches a page
>> -	again, we repeat the same scheme */
>> -
>> -	file_update_time(vmf->vma->vm_file);
>> -
>> -	/* protect against the workqueue changing the page list */
>> -	mutex_lock(&fbdefio->lock);
>> -
>>   	/* first write in this cycle, notify the driver */
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 */
>>   	lock_page(pageref->page);
>>   
>> -	mutex_unlock(&fbdefio->lock);
>> -
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	return ret;
>>   }
>>   
>> +/*
>> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
>> + * @fb_info: The fbdev info structure
>> + * @vmf: The VM fault
>> + *
>> + * This is a callback we get when userspace first tries to
>> + * write to the page. We schedule a workqueue. That workqueue
>> + * will eventually mkclean the touched pages and execute the
>> + * deferred framebuffer IO. Then if userspace touches a page
>> + * again, we repeat the same scheme.
>> + *
>> + * Returns:
>> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
>> + */
>> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
>> +{
>> +	struct page *page = vmf->page;
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>> +
>> +	file_update_time(vmf->vma->vm_file);
>> +
>> +	/* protect against the workqueue changing the page list */
>> +	mutex_lock(&fbdefio->lock);
>> +	ret = __fb_deferred_io_track_page(info, offset, page);
>> +	mutex_unlock(&fbdefio->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/* vm_ops->page_mkwrite handler */
>> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +{
>> +	struct fb_info *info = vmf->vma->vm_private_data;
>> +
>> +	return fb_deferred_io_page_mkwrite(info, vmf);
>> +}
>> +
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>>   	.fault		= fb_deferred_io_fault,
>>   	.page_mkwrite	= fb_deferred_io_mkwrite,
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5100 bytes --]

Hi

Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
>> Refactor the page-write handler for deferred I/O. Drivers use the
>> function to let fbdev track written pages of mmap'ed framebuffer
>> memory.
> 
> I like how the comments got a brush up and a little more info was added.
> But I do not see the point of the refactoring - the code is equally
> readable before and after - maybe even easier before (modulus the
> improved comments).

FYI I'm going to move the locking into the track-page helper, which 
makes the code more readable.

Best regards
Thomas

> 
> But if you consider it better keep it. Again just my thoughts when
> reading the code.
> 
> 	Sam
> 
>>
>> v2:
>> 	* don't export the helper until we have an external caller
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index a03b9c64fc61..214581ce5840 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>>   
>> -/* vm_ops->page_mkwrite handler */
>> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +/*
>> + * Adds a page to the dirty list. Requires caller to hold
>> + * struct fb_deferred_io.lock. Call this from struct
>> + * vm_operations_struct.page_mkwrite.
>> + */
>> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
>> +					      struct page *page)
>>   {
>> -	struct page *page = vmf->page;
>> -	struct fb_info *info = vmf->vma->vm_private_data;
>>   	struct fb_deferred_io *fbdefio = info->fbdefio;
>>   	struct fb_deferred_io_pageref *pageref;
>> -	unsigned long offset;
>>   	vm_fault_t ret;
>>   
>> -	offset = (vmf->address - vmf->vma->vm_start);
>> -
>> -	/* this is a callback we get when userspace first tries to
>> -	write to the page. we schedule a workqueue. that workqueue
>> -	will eventually mkclean the touched pages and execute the
>> -	deferred framebuffer IO. then if userspace touches a page
>> -	again, we repeat the same scheme */
>> -
>> -	file_update_time(vmf->vma->vm_file);
>> -
>> -	/* protect against the workqueue changing the page list */
>> -	mutex_lock(&fbdefio->lock);
>> -
>>   	/* first write in this cycle, notify the driver */
>>   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>>   		fbdefio->first_io(info);
>> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	 */
>>   	lock_page(pageref->page);
>>   
>> -	mutex_unlock(&fbdefio->lock);
>> -
>>   	/* come back after delay to process the deferred IO */
>>   	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>>   	return VM_FAULT_LOCKED;
>> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>>   	return ret;
>>   }
>>   
>> +/*
>> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
>> + * @fb_info: The fbdev info structure
>> + * @vmf: The VM fault
>> + *
>> + * This is a callback we get when userspace first tries to
>> + * write to the page. We schedule a workqueue. That workqueue
>> + * will eventually mkclean the touched pages and execute the
>> + * deferred framebuffer IO. Then if userspace touches a page
>> + * again, we repeat the same scheme.
>> + *
>> + * Returns:
>> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
>> + */
>> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
>> +{
>> +	struct page *page = vmf->page;
>> +	struct fb_deferred_io *fbdefio = info->fbdefio;
>> +	unsigned long offset;
>> +	vm_fault_t ret;
>> +
>> +	offset = (vmf->address - vmf->vma->vm_start);
>> +
>> +	file_update_time(vmf->vma->vm_file);
>> +
>> +	/* protect against the workqueue changing the page list */
>> +	mutex_lock(&fbdefio->lock);
>> +	ret = __fb_deferred_io_track_page(info, offset, page);
>> +	mutex_unlock(&fbdefio->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/* vm_ops->page_mkwrite handler */
>> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> +{
>> +	struct fb_info *info = vmf->vma->vm_private_data;
>> +
>> +	return fb_deferred_io_page_mkwrite(info, vmf);
>> +}
>> +
>>   static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>>   	.fault		= fb_deferred_io_fault,
>>   	.page_mkwrite	= fb_deferred_io_mkwrite,
>> -- 
>> 2.36.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Sam Ravnborg]

Hi Thomas,

> > > +
> > >   /* this is to find and return the vmalloc-ed fb pages */
> > >   static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> > >   {
> > > @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> > >   		printk(KERN_ERR "no mapping available\n");
> > >   	BUG_ON(!page->mapping);
> > > -	page->index = vmf->pgoff;
> > > +	page->index = vmf->pgoff; /* for page_mkclean() */
> > >   	vmf->page = page;
> > >   	return 0;
> > > @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> > >   	struct page *page = vmf->page;
> > >   	struct fb_info *info = vmf->vma->vm_private_data;
> > >   	struct fb_deferred_io *fbdefio = info->fbdefio;
> > > -	struct list_head *pos = &fbdefio->pagelist;
> > > +	struct fb_deferred_io_pageref *pageref;
> > > +	unsigned long offset;
> > > +	vm_fault_t ret;
> > > +
> > > +	offset = (vmf->address - vmf->vma->vm_start);
> > >   	/* this is a callback we get when userspace first tries to
> > >   	write to the page. we schedule a workqueue. that workqueue
> > > @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> > >   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
> > >   		fbdefio->first_io(info);
> > > +	pageref = fb_deferred_io_pageref_get(info, offset, page);
> > Compared to the old code we now do all the sorting and stuff without
> > the page locked, which seem like a big change.
> 
> We never touch any of the page's fields in fb_deferred_io_pageref_get().
> It's only used to initialize the pageref's page pointer. The pagerefs are
> all protected by fbdev-internal locking.  Is there a reason why we should
> further hold the page lock?
I only commented because it was a change in scope of the lock, I did not
see anything wrong in the locking, but then I do not understand locking
so that does not say much.

> 
> All sorting is done by the pageref addresses, which implicitly correspond to
> 'offset'. After looking at the new function again, I'll change it to sort
> directly by offset. It's clearer in its intend.
Looks forward for the re-spin.

	Sam

Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct

[Sam Ravnborg]

Hi Thomas,

> > > +
> > >   /* this is to find and return the vmalloc-ed fb pages */
> > >   static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> > >   {
> > > @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> > >   		printk(KERN_ERR "no mapping available\n");
> > >   	BUG_ON(!page->mapping);
> > > -	page->index = vmf->pgoff;
> > > +	page->index = vmf->pgoff; /* for page_mkclean() */
> > >   	vmf->page = page;
> > >   	return 0;
> > > @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> > >   	struct page *page = vmf->page;
> > >   	struct fb_info *info = vmf->vma->vm_private_data;
> > >   	struct fb_deferred_io *fbdefio = info->fbdefio;
> > > -	struct list_head *pos = &fbdefio->pagelist;
> > > +	struct fb_deferred_io_pageref *pageref;
> > > +	unsigned long offset;
> > > +	vm_fault_t ret;
> > > +
> > > +	offset = (vmf->address - vmf->vma->vm_start);
> > >   	/* this is a callback we get when userspace first tries to
> > >   	write to the page. we schedule a workqueue. that workqueue
> > > @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> > >   	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
> > >   		fbdefio->first_io(info);
> > > +	pageref = fb_deferred_io_pageref_get(info, offset, page);
> > Compared to the old code we now do all the sorting and stuff without
> > the page locked, which seem like a big change.
> 
> We never touch any of the page's fields in fb_deferred_io_pageref_get().
> It's only used to initialize the pageref's page pointer. The pagerefs are
> all protected by fbdev-internal locking.  Is there a reason why we should
> further hold the page lock?
I only commented because it was a change in scope of the lock, I did not
see anything wrong in the locking, but then I do not understand locking
so that does not say much.

> 
> All sorting is done by the pageref addresses, which implicitly correspond to
> 'offset'. After looking at the new function again, I'll change it to sort
> directly by offset. It's clearer in its intend.
Looks forward for the re-spin.

	Sam