On Wed, Nov 27, 2019 at 07:01:16PM +0200, Ville Syrjälä wrote:

On Wed, Nov 27, 2019 at 06:31:57PM +0200, Jani Nikula wrote:

quoted

Modifying fb_ops directly to override fb_mmap with fb_deferred_io_mmap
and then resetting it to NULL afterwards causes problems all over the
place. First, it prevents making the fbops member of struct fb_info a
const pointer, which means we can't make struct fb_ops const
anywhere. Second, a few places have to go out of their way to restore
the original fb_mmap pointer that gets reset to NULL.

Preserve the passed in fb_ops by making a copy of it and modifying that
instead. Add a deferred_io_private member to struct fb_info to store the
pointer to the old fb_ops, and restore that at cleanup.

Quite the horror show. I wonder how hard it would be to just have each
driver that can use defio provide a mmap hook that either dispatches
to the defio one or the native one depending if defio is used or not...

A few drivers at least seem to always use defio so those should be
trivial. Some others seem to make the decision dynamically, which
would require custom .mmap(). Though I suppose all of them could just
be of the form
foo_mmap_wrap{)
{
	if (info->defio)
		defio_mmap()
	else
		foo_mmap();
}

Hmm. Actually is .fb_mmap() called from anywhere but fb_mmap()? If not
we could just shove the defio check there? Or if it's called from
several places we could try to wrap all calls in _fb_mmap() or somesuch.

While we're spinning this garn about fb_mmap improvements: My cunning
plan, documented in todo.rst even, is to have our own fb_mmap
implementation for the drm fbdev helper, which will handle all this
internally.

And then just quietly burn down the fbdev defio stuff to the ground,
because it's the stuff for nightmares.

https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support

tldr; Imo no point investing too much in polishing the defio stuff.
-Daniel

quoted

Cc: Jaya Kumar <jayalk@intworks.biz>
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Jani Nikula <redacted>

---

Note: If the approach is acceptable, we'll also need to handle the error
returns on memory allocation failures at fb_deferred_io_init() call
sites. There are 13.
---
 drivers/video/fbdev/core/fb_defio.c | 25 ++++++++++++++++++++++---
 include/linux/fb.h                  |  3 ++-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 82c20c6047b0..36697844c1e0 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c

@@ -200,13 +200,23 @@ static void fb_deferred_io_work(struct work_struct *work)
 	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 fb_ops *fbops;
 
 	BUG_ON(!fbdefio);
+
+	fbops = kmemdup(info->fbops, sizeof(*fbops), GFP_KERNEL);
+	if (!fbops)
+		return -ENOMEM;
+
+	fbops->fb_mmap = fb_deferred_io_mmap;
+	info->deferred_io_private = info->fbops;
+	info->fbops = fbops;
+
 	mutex_init(&fbdefio->lock);
-	info->fbops->fb_mmap = fb_deferred_io_mmap;
+
 	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 */

@@ -229,6 +239,12 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	int i;
 
 	BUG_ON(!fbdefio);
+
+	/* sanity check against misuse */
+	if (WARN_ON(!info->deferred_io_private ||
+		    info->fbops->fb_mmap != fb_deferred_io_mmap))
+		return;
+
 	cancel_delayed_work_sync(&info->deferred_work);
 
 	/* clear out the mapping that we setup */

@@ -237,7 +253,10 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 		page->mapping = NULL;
 	}
 
-	info->fbops->fb_mmap = NULL;
+	kfree(info->fbops);
+	info->fbops = info->deferred_io_private;
+	info->deferred_io_private = NULL;
+
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);

diff --git a/include/linux/fb.h b/include/linux/fb.h
index a6ad528990de..65f2abd47745 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h

@@ -470,6 +470,7 @@ struct fb_info {
 #ifdef CONFIG_FB_DEFERRED_IO
 	struct delayed_work deferred_work;
 	struct fb_deferred_io *fbdefio;
+	void *deferred_io_private;
 #endif
 
 	struct fb_ops *fbops;

@@ -658,7 +659,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.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch