[PATCH 0/7] Add drm_fb_helper_fbdev_setup/teardown()

[PATCH 0/7] Add drm_fb_helper_fbdev_setup/teardown()

[Noralf Trønnes]

Hi,

This is a new attempt at simplifying fbdev setup/teardown in drivers.
Hopefully this one is better than my previous attempt. The previous
version allocated the drm_fb_helper struct as well, this time I leave it
to the driver.

I've included an fbdev deferred I/O setup helper as well since it's part
of the teardown.

As always I struggle with documentation so I welcome all help in getting
the words right.

Noralf.

Noralf Trønnes (7):
  drm/fb-helper: Set/clear dev->fb_helper in dummy init/fini
  drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()
  drm/docs: Add todo entry for drm_fb_helper_fbdev_setup()
  drm/fb-helper: Update DOC with new helpers
  drm/fb-helper: Add drm_fb_helper_defio_init()
  drm/fb-cma-helper: Use drm_fb_helper_fbdev_setup/teardown()
  drm/fb-cma-helper: Honour DRM_FBDEV_EMULATION

 Documentation/gpu/todo.rst          |  18 ++++
 drivers/gpu/drm/Kconfig             |   5 +-
 drivers/gpu/drm/drm_fb_cma_helper.c | 111 +++-------------------
 drivers/gpu/drm/drm_fb_helper.c     | 181 +++++++++++++++++++++++++++++++++---
 include/drm/drm_fb_helper.h         |  38 ++++++++
 5 files changed, 241 insertions(+), 112 deletions(-)

-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/7] drm/fb-helper: Set/clear dev->fb_helper in dummy init/fini

[Noralf Trønnes]

Set dev->fb_helper even when fbdev emulation is compiled out,
so drivers can use it to free the structure.
Clear it for consistency.

Fixes: 29ad20b22c8f ("drm: Add drm_device->fb_helper pointer")
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 include/drm/drm_fb_helper.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 1494b12a984a..1bd624579db7 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -33,6 +33,7 @@
 struct drm_fb_helper;
 
 #include <drm/drm_crtc.h>
+#include <drm/drm_device.h>
 #include <linux/kgdb.h>
 
 enum mode_set_atomic {
@@ -332,11 +333,17 @@ static inline int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *helper,
 		       int max_conn)
 {
+	/* So drivers can use it to free the struct */
+	helper->dev = dev;
+	dev->fb_helper = helper;
+
 	return 0;
 }
 
 static inline void drm_fb_helper_fini(struct drm_fb_helper *helper)
 {
+	if (helper && helper->dev)
+		helper->dev->fb_helper = NULL;
 }
 
 static inline int drm_fb_helper_blank(int blank, struct fb_info *info)
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()

[Noralf Trønnes]

Add helpers to setup and teardown fbdev emulation.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 106 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_helper.h     |  25 ++++++++++
 2 files changed, 131 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 04a3a5ce370a..823fc8f50d85 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2729,6 +2729,112 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
+/**
+ * drm_fb_helper_fbdev_setup() - Setup fbdev emulation
+ * @dev: DRM device
+ * @fb_helper: fbdev helper structure to set up
+ * @funcs: fbdev helper functions
+ * @preferred_bpp: Preferred bits per pixel for the device.
+ *                 @dev->mode_config.preferred_depth is used if this is zero.
+ * @max_conn_count: Maximum number of connectors.
+ *                  @dev->mode_config.num_connector is used if this is zero.
+ *
+ * This function sets up fbdev emulation and registers fbdev for access by
+ * userspace. If all connectors are disconnected, setup is deferred to the next
+ * time drm_fb_helper_hotplug_event() is called.
+ * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
+ * function.
+ *
+ * See also: drm_fb_helper_initial_config()
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_fb_helper_fbdev_setup(struct drm_device *dev,
+			      struct drm_fb_helper *fb_helper,
+			      const struct drm_fb_helper_funcs *funcs,
+			      unsigned int preferred_bpp,
+			      unsigned int max_conn_count)
+{
+	int ret;
+
+	if (!preferred_bpp)
+		preferred_bpp = dev->mode_config.preferred_depth;
+	if (!preferred_bpp)
+		preferred_bpp = 32;
+
+	if (!max_conn_count)
+		max_conn_count = dev->mode_config.num_connector;
+	if (!max_conn_count) {
+		DRM_DEV_ERROR(dev->dev, "No connectors\n");
+		return -EINVAL;
+	}
+
+	drm_fb_helper_prepare(dev, fb_helper, funcs);
+
+	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper\n");
+		return ret;
+	}
+
+	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev->dev, "Failed to add connectors\n");
+		goto err_drm_fb_helper_fini;
+	}
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		drm_helper_disable_unused_functions(dev);
+
+	ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration\n");
+		goto err_drm_fb_helper_fini;
+	}
+
+	return 0;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(fb_helper);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
+
+/**
+ * drm_fb_helper_fbdev_teardown - Tear down fbdev emulation
+ * @dev: DRM device
+ *
+ * This function unregisters fbdev if not already done and cleans up the
+ * associated resources including the &drm_framebuffer.
+ * The driver is responsible for freeing the &drm_fb_helper structure which is
+ * stored in &drm_device->fb_helper. Do note that this pointer has been cleared
+ * when this function returns.
+ *
+ * In order to support device removal/unplug while file handles are still open,
+ * drm_fb_helper_unregister_fbi() should be called on device removal and
+ * drm_fb_helper_fbdev_teardown() in the &drm_driver->release callback when
+ * file handles are closed.
+ */
+void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
+{
+	struct drm_fb_helper *fb_helper = dev->fb_helper;
+
+	if (!fb_helper)
+		return;
+
+	/* Unregister if it hasn't been done already */
+	if (fb_helper->fbdev && fb_helper->fbdev->dev)
+		drm_fb_helper_unregister_fbi(fb_helper);
+
+	drm_fb_helper_fini(fb_helper);
+
+	if (fb_helper->fb)
+		drm_framebuffer_remove(fb_helper->fb);
+}
+EXPORT_SYMBOL(drm_fb_helper_fbdev_teardown);
+
 /**
  * drm_fb_helper_lastclose - DRM driver lastclose helper for fbdev emulation
  * @dev: DRM device
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 1bd624579db7..aa78ac3b8ad0 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -320,6 +320,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 				       struct drm_connector *connector);
 
+int drm_fb_helper_fbdev_setup(struct drm_device *dev,
+			      struct drm_fb_helper *fb_helper,
+			      const struct drm_fb_helper_funcs *funcs,
+			      unsigned int preferred_bpp,
+			      unsigned int max_conn_count);
+void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
+
 void drm_fb_helper_lastclose(struct drm_device *dev);
 void drm_fb_helper_output_poll_changed(struct drm_device *dev);
 #else
@@ -525,6 +532,24 @@ drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+static inline int
+drm_fb_helper_fbdev_setup(struct drm_device *dev,
+			  struct drm_fb_helper *fb_helper,
+			  const struct drm_fb_helper_funcs *funcs,
+			  unsigned int preferred_bpp,
+			  unsigned int max_conn_count)
+{
+	/* So drivers can use it to free the struct */
+	dev->fb_helper = fb_helper;
+
+	return 0;
+}
+
+static inline void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
+{
+	dev->fb_helper = NULL;
+}
+
 static inline void drm_fb_helper_lastclose(struct drm_device *dev)
 {
 }
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/7] drm/docs: Add todo entry for drm_fb_helper_fbdev_setup()

[Noralf Trønnes]

Add entry for conversion of drivers to new helpers.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/gpu/todo.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index f421a54527d2..1e593370f64f 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -194,6 +194,24 @@ drm_mode_config_helper_suspend/resume().
 
 Contact: Maintainer of the driver you plan to convert
 
+Convert drivers to use drm_fb_helper_fbdev_setup/teardown()
+-----------------------------------------------------------
+
+Most drivers can use drm_fb_helper_fbdev_setup() except maybe:
+
+- amdgpu which has special logic to decide whether to call
+  drm_helper_disable_unused_functions()
+
+- armada which isn't atomic and doesn't call
+  drm_helper_disable_unused_functions()
+
+- i915 which calls drm_fb_helper_initial_config() in a worker
+
+Drivers that use drm_framebuffer_remove() to clean up the fbdev framebuffer can
+probably use drm_fb_helper_fbdev_teardown().
+
+Contact: Maintainer of the driver you plan to convert
+
 Core refactorings
 =================
 
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/fb-helper: Update DOC with new helpers

[Noralf Trønnes]

Promote new helpers for dealing with fbdev emulation.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 823fc8f50d85..14aa83579e76 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -66,19 +66,17 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
  *
- * Initialization is done as a four-step process with drm_fb_helper_prepare(),
- * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
- * drm_fb_helper_initial_config(). Drivers with fancier requirements than the
- * default behaviour can override the third step with their own code.
- * Teardown is done with drm_fb_helper_fini() after the fbdev device is
- * unregisters using drm_fb_helper_unregister_fbi().
+ * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
+ * down by calling drm_fb_helper_fbdev_teardown().
  *
- * At runtime drivers should restore the fbdev console by calling
- * drm_fb_helper_restore_fbdev_mode_unlocked() from their &drm_driver.lastclose
- * callback.  They should also notify the fb helper code from updates to the
- * output configuration by calling drm_fb_helper_hotplug_event(). For easier
- * integration with the output polling code in drm_crtc_helper.c the modeset
- * code provides a &drm_mode_config_funcs.output_poll_changed callback.
+ * At runtime drivers should restore the fbdev console by using
+ * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
+ * They should also notify the fb helper code from updates to the output
+ * configuration by using drm_fb_helper_output_poll_changed() as their
+ * &drm_mode_config_funcs.output_poll_changed callback.
+ *
+ * For suspend/resume consider using drm_mode_config_helper_suspend() and
+ * drm_mode_config_helper_resume() which takes care of fbdev as well.
  *
  * All other functions exported by the fb helper library can be used to
  * implement the fbdev driver interface by the driver.
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Noralf Trønnes]

Add helper for initializing fbdev deferred I/O.

The cleanup could have happened in drm_fb_helper_fini(), but that would
have required me to set fb_info->fbdefio to NULL in a couple of drivers
before they call _fini() to avoid double defio cleanup. The problem is
that one of those is vboxvideo which lives in Greg's staging tree.
So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
but not that bad either.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_helper.h     |  6 +++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 14aa83579e76..d5eeed1c7749 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
+/**
+ * drm_fb_helper_defio_init - fbdev deferred I/O initialization
+ * @fb_helper: driver-allocated fbdev helper
+ *
+ * This function allocates &fb_deferred_io, sets callback to
+ * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
+ * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
+ *
+ * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
+ * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
+ * affect other instances of that &fb_ops.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
+{
+	struct fb_info *info = fb_helper->fbdev;
+	struct fb_deferred_io *fbdefio;
+	struct fb_ops *fbops;
+
+	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
+	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
+	if (!fbdefio || !fbops) {
+		kfree(fbdefio);
+		kfree(fbops);
+		return -ENOMEM;
+	}
+
+	info->fbdefio = fbdefio;
+	fbdefio->delay = msecs_to_jiffies(50);
+	fbdefio->deferred_io = drm_fb_helper_deferred_io;
+
+	*fbops = *info->fbops;
+	info->fbops = fbops;
+
+	fb_deferred_io_init(info);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_defio_init);
+
 /**
  * drm_fb_helper_sys_read - wrapper around fb_sys_read
  * @info: fb_info struct pointer
@@ -2743,6 +2785,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
  * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
  * function.
  *
+ * Drivers that need fbdev deferred I/O should use drm_fb_helper_defio_init()
+ * to set it up.
+ *
  * See also: drm_fb_helper_initial_config()
  *
  * Returns:
@@ -2818,6 +2863,7 @@ EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
 void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
 {
 	struct drm_fb_helper *fb_helper = dev->fb_helper;
+	struct fb_ops *fbops = NULL;
 
 	if (!fb_helper)
 		return;
@@ -2826,7 +2872,14 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
 	if (fb_helper->fbdev && fb_helper->fbdev->dev)
 		drm_fb_helper_unregister_fbi(fb_helper);
 
+	if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) {
+		fb_deferred_io_cleanup(fb_helper->fbdev);
+		kfree(fb_helper->fbdev->fbdefio);
+		fbops = fb_helper->fbdev->fbops;
+	}
+
 	drm_fb_helper_fini(fb_helper);
+	kfree(fbops);
 
 	if (fb_helper->fb)
 		drm_framebuffer_remove(fb_helper->fb);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index aa78ac3b8ad0..b069433e7fc1 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -276,6 +276,7 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
 
 void drm_fb_helper_deferred_io(struct fb_info *info,
 			       struct list_head *pagelist);
+int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
 
 ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
 			       size_t count, loff_t *ppos);
@@ -423,6 +424,11 @@ static inline void drm_fb_helper_deferred_io(struct fb_info *info,
 {
 }
 
+static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
+{
+	return -ENODEV;
+}
+
 static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
 					     char __user *buf, size_t count,
 					     loff_t *ppos)
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/7] drm/fb-cma-helper: Use drm_fb_helper_fbdev_setup/teardown()

[Noralf Trønnes]

Use the new setup/teardown helpers as well as drm_fb_helper_defio_init().
Wrap fb_deferred_io_mmap() in ifdef so we can make fbdev optional.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

I had to touch drm_fbdev_cma_fini() here, but that will be rebased away
when I this patch is unblocked and applied:
[PATCH v3 11/11] drm/cma-helper: Remove drm_fbdev_cma* functions


 drivers/gpu/drm/drm_fb_cma_helper.c | 111 +++++-------------------------------
 1 file changed, 15 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 186d00adfb5f..9f8210130dc6 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -26,8 +26,6 @@
 #include <drm/drm_print.h>
 #include <linux/module.h>
 
-#define DEFAULT_FBDEFIO_DELAY_MS 50
-
 struct drm_fbdev_cma {
 	struct drm_fb_helper	fb_helper;
 	const struct drm_framebuffer_funcs *fb_funcs;
@@ -149,58 +147,14 @@ static struct fb_ops drm_fbdev_cma_ops = {
 static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
 					  struct vm_area_struct *vma)
 {
+#ifdef CONFIG_FB_DEFERRED_IO
 	fb_deferred_io_mmap(info, vma);
+#endif
 	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
 
 	return 0;
 }
 
-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
-				    struct drm_gem_cma_object *cma_obj)
-{
-	struct fb_deferred_io *fbdefio;
-	struct fb_ops *fbops;
-
-	/*
-	 * Per device structures are needed because:
-	 * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
-	 * fbdefio: individual delays
-	 */
-	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-	if (!fbdefio || !fbops) {
-		kfree(fbdefio);
-		kfree(fbops);
-		return -ENOMEM;
-	}
-
-	/* can't be offset from vaddr since dirty() uses cma_obj */
-	fbi->screen_buffer = cma_obj->vaddr;
-	/* fb_deferred_io_fault() needs a physical address */
-	fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
-
-	*fbops = *fbi->fbops;
-	fbi->fbops = fbops;
-
-	fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
-	fbdefio->deferred_io = drm_fb_helper_deferred_io;
-	fbi->fbdefio = fbdefio;
-	fb_deferred_io_init(fbi);
-	fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
-
-	return 0;
-}
-
-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
-{
-	if (!fbi->fbdefio)
-		return;
-
-	fb_deferred_io_cleanup(fbi);
-	kfree(fbi->fbdefio);
-	kfree(fbi->fbops);
-}
-
 static int
 drm_fbdev_cma_create(struct drm_fb_helper *helper,
 	struct drm_fb_helper_surface_size *sizes)
@@ -258,9 +212,15 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper,
 	fbi->fix.smem_len = size;
 
 	if (fb->funcs->dirty) {
-		ret = drm_fbdev_cma_defio_init(fbi, obj);
+		ret = drm_fb_helper_defio_init(helper);
 		if (ret)
 			goto err_cma_destroy;
+
+		/* can't be offset from vaddr since dirty() uses cma obj */
+		fbi->screen_buffer = obj->vaddr;
+		/* fb_deferred_io_fault() needs a physical address */
+		fbi->fix.smem_start = page_to_phys(virt_to_page(obj->vaddr));
+		fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
 	}
 
 	return 0;
@@ -296,52 +256,23 @@ int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev,
 	const struct drm_framebuffer_funcs *funcs)
 {
 	struct drm_fbdev_cma *fbdev_cma;
-	struct drm_fb_helper *fb_helper;
 	int ret;
 
-	if (!preferred_bpp)
-		preferred_bpp = dev->mode_config.preferred_depth;
-	if (!preferred_bpp)
-		preferred_bpp = 32;
-
-	if (!max_conn_count)
-		max_conn_count = dev->mode_config.num_connector;
-
 	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
 	if (!fbdev_cma)
 		return -ENOMEM;
 
 	fbdev_cma->fb_funcs = funcs;
-	fb_helper = &fbdev_cma->fb_helper;
 
-	drm_fb_helper_prepare(dev, fb_helper, &drm_fb_cma_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
+	ret = drm_fb_helper_fbdev_setup(dev, &fbdev_cma->fb_helper,
+					&drm_fb_cma_helper_funcs,
+					preferred_bpp, max_conn_count);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper.\n");
-		goto err_free;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
-		goto err_drm_fb_helper_fini;
-	}
-
-	ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration.\n");
-		goto err_drm_fb_helper_fini;
+		kfree(fbdev_cma);
+		return ret;
 	}
 
 	return 0;
-
-err_drm_fb_helper_fini:
-	drm_fb_helper_fini(fb_helper);
-err_free:
-	kfree(fbdev_cma);
-
-	return ret;
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs);
 
@@ -375,17 +306,7 @@ void drm_fb_cma_fbdev_fini(struct drm_device *dev)
 	if (!fb_helper)
 		return;
 
-	/* Unregister if it hasn't been done already */
-	if (fb_helper->fbdev && fb_helper->fbdev->dev)
-		drm_fb_helper_unregister_fbi(fb_helper);
-
-	if (fb_helper->fbdev)
-		drm_fbdev_cma_defio_fini(fb_helper->fbdev);
-
-	if (fb_helper->fb)
-		drm_framebuffer_remove(fb_helper->fb);
-
-	drm_fb_helper_fini(fb_helper);
+	drm_fb_helper_fbdev_teardown(dev);
 	kfree(to_fbdev_cma(fb_helper));
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_fini);
@@ -477,8 +398,6 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
 void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
 {
 	drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
-	if (fbdev_cma->fb_helper.fbdev)
-		drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
 
 	if (fbdev_cma->fb_helper.fb)
 		drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/fb-cma-helper: Honour DRM_FBDEV_EMULATION

[Noralf Trønnes]

Make it possible to opt out of fbdev emulation.
DRM_FBDEV_EMULATION selects DRM_KMS_FB_HELPER and is default yes so
this doesn't change the default behaviour.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Kconfig | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index d853989848d6..fe1b6030d3c6 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -143,10 +143,7 @@ config DRM_KMS_CMA_HELPER
 	bool
 	depends on DRM
 	select DRM_GEM_CMA_HELPER
-	select DRM_KMS_FB_HELPER
-	select FB_SYS_FILLRECT
-	select FB_SYS_COPYAREA
-	select FB_SYS_IMAGEBLIT
+	select DRM_KMS_HELPER
 	help
 	  Choose this if you need the KMS CMA helper functions
 
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/fb-helper: Set/clear dev->fb_helper in dummy init/fini

[Daniel Vetter]

On Thu, Dec 14, 2017 at 08:10:02PM +0100, Noralf Trønnes wrote:
> Set dev->fb_helper even when fbdev emulation is compiled out,
> so drivers can use it to free the structure.
> Clear it for consistency.
> 
> Fixes: 29ad20b22c8f ("drm: Add drm_device->fb_helper pointer")
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  include/drm/drm_fb_helper.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 1494b12a984a..1bd624579db7 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -33,6 +33,7 @@
>  struct drm_fb_helper;
>  
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_device.h>
>  #include <linux/kgdb.h>
>  
>  enum mode_set_atomic {
> @@ -332,11 +333,17 @@ static inline int drm_fb_helper_init(struct drm_device *dev,
>  		       struct drm_fb_helper *helper,
>  		       int max_conn)
>  {
> +	/* So drivers can use it to free the struct */
> +	helper->dev = dev;
> +	dev->fb_helper = helper;

Not sure I forgot it, or it got dropped, but I'd like
dev->mode_config.fb_helper better. That's where all the other kms stuff
resides.

But 100% bikeshed, so feel free to ignore. Definitely only a follow-up
patch.
-Daniel

> +
>  	return 0;
>  }
>  
>  static inline void drm_fb_helper_fini(struct drm_fb_helper *helper)
>  {
> +	if (helper && helper->dev)
> +		helper->dev->fb_helper = NULL;
>  }
>  
>  static inline int drm_fb_helper_blank(int blank, struct fb_info *info)
> -- 
> 2.14.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()

[Daniel Vetter]

On Thu, Dec 14, 2017 at 08:10:03PM +0100, Noralf Trønnes wrote:
> Add helpers to setup and teardown fbdev emulation.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 106 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h     |  25 ++++++++++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 04a3a5ce370a..823fc8f50d85 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2729,6 +2729,112 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>  
> +/**
> + * drm_fb_helper_fbdev_setup() - Setup fbdev emulation
> + * @dev: DRM device
> + * @fb_helper: fbdev helper structure to set up
> + * @funcs: fbdev helper functions
> + * @preferred_bpp: Preferred bits per pixel for the device.
> + *                 @dev->mode_config.preferred_depth is used if this is zero.
> + * @max_conn_count: Maximum number of connectors.
> + *                  @dev->mode_config.num_connector is used if this is zero.
> + *
> + * This function sets up fbdev emulation and registers fbdev for access by
> + * userspace. If all connectors are disconnected, setup is deferred to the next
> + * time drm_fb_helper_hotplug_event() is called.
> + * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
> + * function.

I think we should be a bit clearer when it's not advisable to use this
helper because there are issues:
- when connectors can be hotplugged (e.g. dp mst). For that case you need
  to init the fbdev before you enabling hotplugging, but call
  initial_config after you've enabled hotplugging. Otherwise there's a
  posssible race window.

- when there's anything else that mus be done before we do the fbdev
  registration, e.g. defio_init.

So still not 100% sure this will help you all that much, but it's also not
an unreasonable idea.

Assuming we can agree on the ceveats and how do document them:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> + *
> + * See also: drm_fb_helper_initial_config()
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_fbdev_setup(struct drm_device *dev,
> +			      struct drm_fb_helper *fb_helper,
> +			      const struct drm_fb_helper_funcs *funcs,
> +			      unsigned int preferred_bpp,
> +			      unsigned int max_conn_count)
> +{
> +	int ret;
> +
> +	if (!preferred_bpp)
> +		preferred_bpp = dev->mode_config.preferred_depth;
> +	if (!preferred_bpp)
> +		preferred_bpp = 32;
> +
> +	if (!max_conn_count)
> +		max_conn_count = dev->mode_config.num_connector;
> +	if (!max_conn_count) {
> +		DRM_DEV_ERROR(dev->dev, "No connectors\n");
> +		return -EINVAL;
> +	}
> +
> +	drm_fb_helper_prepare(dev, fb_helper, funcs);
> +
> +	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper\n");
> +		return ret;
> +	}
> +
> +	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to add connectors\n");
> +		goto err_drm_fb_helper_fini;
> +	}
> +
> +	if (!drm_drv_uses_atomic_modeset(dev))
> +		drm_helper_disable_unused_functions(dev);
> +
> +	ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration\n");
> +		goto err_drm_fb_helper_fini;
> +	}
> +
> +	return 0;
> +
> +err_drm_fb_helper_fini:
> +	drm_fb_helper_fini(fb_helper);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
> +
> +/**
> + * drm_fb_helper_fbdev_teardown - Tear down fbdev emulation
> + * @dev: DRM device
> + *
> + * This function unregisters fbdev if not already done and cleans up the
> + * associated resources including the &drm_framebuffer.
> + * The driver is responsible for freeing the &drm_fb_helper structure which is
> + * stored in &drm_device->fb_helper. Do note that this pointer has been cleared
> + * when this function returns.
> + *
> + * In order to support device removal/unplug while file handles are still open,
> + * drm_fb_helper_unregister_fbi() should be called on device removal and
> + * drm_fb_helper_fbdev_teardown() in the &drm_driver->release callback when
> + * file handles are closed.
> + */
> +void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
> +{
> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
> +
> +	if (!fb_helper)
> +		return;
> +
> +	/* Unregister if it hasn't been done already */
> +	if (fb_helper->fbdev && fb_helper->fbdev->dev)
> +		drm_fb_helper_unregister_fbi(fb_helper);
> +
> +	drm_fb_helper_fini(fb_helper);
> +
> +	if (fb_helper->fb)
> +		drm_framebuffer_remove(fb_helper->fb);
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fbdev_teardown);
> +
>  /**
>   * drm_fb_helper_lastclose - DRM driver lastclose helper for fbdev emulation
>   * @dev: DRM device
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 1bd624579db7..aa78ac3b8ad0 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -320,6 +320,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  				       struct drm_connector *connector);
>  
> +int drm_fb_helper_fbdev_setup(struct drm_device *dev,
> +			      struct drm_fb_helper *fb_helper,
> +			      const struct drm_fb_helper_funcs *funcs,
> +			      unsigned int preferred_bpp,
> +			      unsigned int max_conn_count);
> +void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
> +
>  void drm_fb_helper_lastclose(struct drm_device *dev);
>  void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>  #else
> @@ -525,6 +532,24 @@ drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	return 0;
>  }
>  
> +static inline int
> +drm_fb_helper_fbdev_setup(struct drm_device *dev,
> +			  struct drm_fb_helper *fb_helper,
> +			  const struct drm_fb_helper_funcs *funcs,
> +			  unsigned int preferred_bpp,
> +			  unsigned int max_conn_count)
> +{
> +	/* So drivers can use it to free the struct */
> +	dev->fb_helper = fb_helper;
> +
> +	return 0;
> +}
> +
> +static inline void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
> +{
> +	dev->fb_helper = NULL;
> +}
> +
>  static inline void drm_fb_helper_lastclose(struct drm_device *dev)
>  {
>  }
> -- 
> 2.14.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Daniel Vetter]

On Thu, Dec 14, 2017 at 08:10:06PM +0100, Noralf Trønnes wrote:
> Add helper for initializing fbdev deferred I/O.
> 
> The cleanup could have happened in drm_fb_helper_fini(), but that would
> have required me to set fb_info->fbdefio to NULL in a couple of drivers
> before they call _fini() to avoid double defio cleanup. The problem is
> that one of those is vboxvideo which lives in Greg's staging tree.
> So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
> but not that bad either.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h     |  6 +++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 14aa83579e76..d5eeed1c7749 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>  
> +/**
> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> + * @fb_helper: driver-allocated fbdev helper
> + *
> + * This function allocates &fb_deferred_io, sets callback to
> + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> + * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> + *
> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> + * affect other instances of that &fb_ops.

Do we need to call this before initial_config? Or after? Should be
documented imo.

Also, did you look into just fixing fb_deferred_io_cleanup to no longer do
this? Changing function tables is definitely not cool (because that means
we can't put them into read-only memory and protect them from attackers -
function tables are a high-value target in the kernel, since usually easy
to trigger them).


> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> +	struct fb_info *info = fb_helper->fbdev;
> +	struct fb_deferred_io *fbdefio;
> +	struct fb_ops *fbops;
> +
> +	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> +	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> +	if (!fbdefio || !fbops) {
> +		kfree(fbdefio);
> +		kfree(fbops);
> +		return -ENOMEM;
> +	}
> +
> +	info->fbdefio = fbdefio;
> +	fbdefio->delay = msecs_to_jiffies(50);
> +	fbdefio->deferred_io = drm_fb_helper_deferred_io;
> +
> +	*fbops = *info->fbops;
> +	info->fbops = fbops;
> +
> +	fb_deferred_io_init(info);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_defio_init);
> +
>  /**
>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
>   * @info: fb_info struct pointer
> @@ -2743,6 +2785,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>   * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>   * function.
>   *
> + * Drivers that need fbdev deferred I/O should use drm_fb_helper_defio_init()
> + * to set it up.

Not exactly sure why you put this here ... Maybe throw it into the
overview documentation, in a new paragraph that explains when you have a
non-NULL fb->dirty callback, then you most likely want to setup
defio_init. Except for the shmem fun. Explaining all that would be good
(maybe even put it under a "Framebuffer Flushing" heading).

Otherwise looks all reasonable to me.

Cheers, Daniel

> + *
>   * See also: drm_fb_helper_initial_config()
>   *
>   * Returns:
> @@ -2818,6 +2863,7 @@ EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
>  void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>  {
>  	struct drm_fb_helper *fb_helper = dev->fb_helper;
> +	struct fb_ops *fbops = NULL;
>  
>  	if (!fb_helper)
>  		return;
> @@ -2826,7 +2872,14 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>  	if (fb_helper->fbdev && fb_helper->fbdev->dev)
>  		drm_fb_helper_unregister_fbi(fb_helper);
>  
> +	if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) {
> +		fb_deferred_io_cleanup(fb_helper->fbdev);
> +		kfree(fb_helper->fbdev->fbdefio);
> +		fbops = fb_helper->fbdev->fbops;
> +	}
> +
>  	drm_fb_helper_fini(fb_helper);
> +	kfree(fbops);
>  
>  	if (fb_helper->fb)
>  		drm_framebuffer_remove(fb_helper->fb);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index aa78ac3b8ad0..b069433e7fc1 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -276,6 +276,7 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>  
>  void drm_fb_helper_deferred_io(struct fb_info *info,
>  			       struct list_head *pagelist);
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>  
>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>  			       size_t count, loff_t *ppos);
> @@ -423,6 +424,11 @@ static inline void drm_fb_helper_deferred_io(struct fb_info *info,
>  {
>  }
>  
> +static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> +	return -ENODEV;
> +}
> +
>  static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
>  					     char __user *buf, size_t count,
>  					     loff_t *ppos)
> -- 
> 2.14.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/fb-cma-helper: Honour DRM_FBDEV_EMULATION

[Daniel Vetter]

On Thu, Dec 14, 2017 at 08:10:08PM +0100, Noralf Trønnes wrote:
> Make it possible to opt out of fbdev emulation.
> DRM_FBDEV_EMULATION selects DRM_KMS_FB_HELPER and is default yes so
> this doesn't change the default behaviour.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

For all the patches I didn't comment on:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/Kconfig | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index d853989848d6..fe1b6030d3c6 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -143,10 +143,7 @@ config DRM_KMS_CMA_HELPER
>  	bool
>  	depends on DRM
>  	select DRM_GEM_CMA_HELPER
> -	select DRM_KMS_FB_HELPER
> -	select FB_SYS_FILLRECT
> -	select FB_SYS_COPYAREA
> -	select FB_SYS_IMAGEBLIT
> +	select DRM_KMS_HELPER
>  	help
>  	  Choose this if you need the KMS CMA helper functions
>  
> -- 
> 2.14.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/fb-helper: Update DOC with new helpers

[Noralf Trønnes]

Den 14.12.2017 20.10, skrev Noralf Trønnes:
> Promote new helpers for dealing with fbdev emulation.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 823fc8f50d85..14aa83579e76 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -66,19 +66,17 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>    * helper functions used by many drivers to implement the kernel mode setting
>    * interfaces.
>    *
> - * Initialization is done as a four-step process with drm_fb_helper_prepare(),
> - * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> - * drm_fb_helper_initial_config(). Drivers with fancier requirements than the
> - * default behaviour can override the third step with their own code.
> - * Teardown is done with drm_fb_helper_fini() after the fbdev device is
> - * unregisters using drm_fb_helper_unregister_fbi().
> + * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> + * down by calling drm_fb_helper_fbdev_teardown().
>    *

How about this for documenting when _setup() can't be used:

  * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
  * down by calling drm_fb_helper_fbdev_teardown().
  *
  * Drivers that need to handle connector hotplugging (e.g. dp mst) 
can't use
  * the setup helper and will need to do the whole four-step setup 
process with
  * drm_fb_helper_prepare(), drm_fb_helper_init(),
  * drm_fb_helper_single_add_all_connectors(), enable hotplugging and
  * drm_fb_helper_initial_config() to avoid a possible race window.

Noralf.

> - * At runtime drivers should restore the fbdev console by calling
> - * drm_fb_helper_restore_fbdev_mode_unlocked() from their &drm_driver.lastclose
> - * callback.  They should also notify the fb helper code from updates to the
> - * output configuration by calling drm_fb_helper_hotplug_event(). For easier
> - * integration with the output polling code in drm_crtc_helper.c the modeset
> - * code provides a &drm_mode_config_funcs.output_poll_changed callback.
> + * At runtime drivers should restore the fbdev console by using
> + * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> + * They should also notify the fb helper code from updates to the output
> + * configuration by using drm_fb_helper_output_poll_changed() as their
> + * &drm_mode_config_funcs.output_poll_changed callback.
> + *
> + * For suspend/resume consider using drm_mode_config_helper_suspend() and
> + * drm_mode_config_helper_resume() which takes care of fbdev as well.
>    *
>    * All other functions exported by the fb helper library can be used to
>    * implement the fbdev driver interface by the driver.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Noralf Trønnes]

Den 14.12.2017 21.25, skrev Daniel Vetter:
> On Thu, Dec 14, 2017 at 08:10:06PM +0100, Noralf Trønnes wrote:
>> Add helper for initializing fbdev deferred I/O.
>>
>> The cleanup could have happened in drm_fb_helper_fini(), but that would
>> have required me to set fb_info->fbdefio to NULL in a couple of drivers
>> before they call _fini() to avoid double defio cleanup. The problem is
>> that one of those is vboxvideo which lives in Greg's staging tree.
>> So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
>> but not that bad either.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_fb_helper.h     |  6 +++++
>>   2 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 14aa83579e76..d5eeed1c7749 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>   
>> +/**
>> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>> + * @fb_helper: driver-allocated fbdev helper
>> + *
>> + * This function allocates &fb_deferred_io, sets callback to
>> + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>> + * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>> + *
>> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>> + * affect other instances of that &fb_ops.
> Do we need to call this before initial_config? Or after? Should be
> documented imo.

Indeed it should be:

  * This function allocates &fb_deferred_io, sets callback to
  * drm_fb_helper_deferred_io(), delay to 50ms and calls 
fb_deferred_io_init().
  * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
  * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.

> Also, did you look into just fixing fb_deferred_io_cleanup to no longer do
> this? Changing function tables is definitely not cool (because that means
> we can't put them into read-only memory and protect them from attackers -
> function tables are a high-value target in the kernel, since usually easy
> to trigger them).

The fbdev operations isn't const:

struct fb_info {
     struct fb_ops *fbops;
};

Fixing that is a project of it's own as a quick grep revealed:

drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_fillrect 
= cfb_fillrect;
drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_copyarea 
= cfb_copyarea;
drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_imageblit 
= cfb_imageblit;
drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_fillrect 
= mb86290fb_fillrect;
drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_copyarea 
= mb86290fb_copyarea;
drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_imageblit 
= mb86290fb_imageblit;

drivers/video/fbdev/uvesafb.c:          info->fbops->fb_blank = NULL;
drivers/video/fbdev/uvesafb.c: info->fbops->fb_pan_display = NULL;

drivers/video/fbdev/aty/atyfb_base.c: info->fbops->fb_sync = atyfb_sync;
drivers/video/fbdev/aty/atyfb_base.c: info->fbops->fb_sync = NULL;

drivers/video/fbdev/aty/mach64_cursor.c: info->fbops->fb_cursor = 
atyfb_cursor;

drivers/video/fbdev/core/fb_defio.c:    info->fbops->fb_mmap = 
fb_deferred_io_mmap;
drivers/video/fbdev/core/fb_defio.c:    info->fbops->fb_mmap = NULL;

drivers/video/fbdev/vesafb.c: info->fbops->fb_pan_display = NULL;

drivers/video/fbdev/udlfb.c:            info->fbops->fb_mmap = 
dlfb_ops_mmap;

drivers/video/fbdev/smscufx.c:          info->fbops->fb_mmap = ufx_ops_mmap;

drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_imageblit = 
nvidiafb_imageblit;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_fillrect = 
nvidiafb_fillrect;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_copyarea = 
nvidiafb_copyarea;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_sync = nvidiafb_sync;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_imageblit = 
cfb_imageblit;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_fillrect = 
cfb_fillrect;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_copyarea = 
cfb_copyarea;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_sync = NULL;
drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_cursor = NULL;

drivers/gpu/drm/udl/udl_fb.c:           info->fbops->fb_mmap = udl_fb_mmap;

drivers/gpu/drm/drm_fb_cma_helper.c: fbi->fbops->fb_mmap = 
drm_fbdev_cma_deferred_io_mmap;

Noralf.

>
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>> +{
>> +	struct fb_info *info = fb_helper->fbdev;
>> +	struct fb_deferred_io *fbdefio;
>> +	struct fb_ops *fbops;
>> +
>> +	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>> +	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>> +	if (!fbdefio || !fbops) {
>> +		kfree(fbdefio);
>> +		kfree(fbops);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	info->fbdefio = fbdefio;
>> +	fbdefio->delay = msecs_to_jiffies(50);
>> +	fbdefio->deferred_io = drm_fb_helper_deferred_io;
>> +
>> +	*fbops = *info->fbops;
>> +	info->fbops = fbops;
>> +
>> +	fb_deferred_io_init(info);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_fb_helper_defio_init);
>> +
>>   /**
>>    * drm_fb_helper_sys_read - wrapper around fb_sys_read
>>    * @info: fb_info struct pointer
>> @@ -2743,6 +2785,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>>    * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>>    * function.
>>    *
>> + * Drivers that need fbdev deferred I/O should use drm_fb_helper_defio_init()
>> + * to set it up.
> Not exactly sure why you put this here ... Maybe throw it into the
> overview documentation, in a new paragraph that explains when you have a
> non-NULL fb->dirty callback, then you most likely want to setup
> defio_init. Except for the shmem fun. Explaining all that would be good
> (maybe even put it under a "Framebuffer Flushing" heading).
>
> Otherwise looks all reasonable to me.
>
> Cheers, Daniel
>
>> + *
>>    * See also: drm_fb_helper_initial_config()
>>    *
>>    * Returns:
>> @@ -2818,6 +2863,7 @@ EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
>>   void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>>   {
>>   	struct drm_fb_helper *fb_helper = dev->fb_helper;
>> +	struct fb_ops *fbops = NULL;
>>   
>>   	if (!fb_helper)
>>   		return;
>> @@ -2826,7 +2872,14 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>>   	if (fb_helper->fbdev && fb_helper->fbdev->dev)
>>   		drm_fb_helper_unregister_fbi(fb_helper);
>>   
>> +	if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) {
>> +		fb_deferred_io_cleanup(fb_helper->fbdev);
>> +		kfree(fb_helper->fbdev->fbdefio);
>> +		fbops = fb_helper->fbdev->fbops;
>> +	}
>> +
>>   	drm_fb_helper_fini(fb_helper);
>> +	kfree(fbops);
>>   
>>   	if (fb_helper->fb)
>>   		drm_framebuffer_remove(fb_helper->fb);
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index aa78ac3b8ad0..b069433e7fc1 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -276,6 +276,7 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>   
>>   void drm_fb_helper_deferred_io(struct fb_info *info,
>>   			       struct list_head *pagelist);
>> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>>   
>>   ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>>   			       size_t count, loff_t *ppos);
>> @@ -423,6 +424,11 @@ static inline void drm_fb_helper_deferred_io(struct fb_info *info,
>>   {
>>   }
>>   
>> +static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>   static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
>>   					     char __user *buf, size_t count,
>>   					     loff_t *ppos)
>> -- 
>> 2.14.2
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()

[Noralf Trønnes]

Den 14.12.2017 21.19, skrev Daniel Vetter:
> On Thu, Dec 14, 2017 at 08:10:03PM +0100, Noralf Trønnes wrote:
>> Add helpers to setup and teardown fbdev emulation.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 106 ++++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_fb_helper.h     |  25 ++++++++++
>>   2 files changed, 131 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 04a3a5ce370a..823fc8f50d85 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2729,6 +2729,112 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>>   
>> +/**
>> + * drm_fb_helper_fbdev_setup() - Setup fbdev emulation
>> + * @dev: DRM device
>> + * @fb_helper: fbdev helper structure to set up
>> + * @funcs: fbdev helper functions
>> + * @preferred_bpp: Preferred bits per pixel for the device.
>> + *                 @dev->mode_config.preferred_depth is used if this is zero.
>> + * @max_conn_count: Maximum number of connectors.
>> + *                  @dev->mode_config.num_connector is used if this is zero.
>> + *
>> + * This function sets up fbdev emulation and registers fbdev for access by
>> + * userspace. If all connectors are disconnected, setup is deferred to the next
>> + * time drm_fb_helper_hotplug_event() is called.
>> + * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>> + * function.
> I think we should be a bit clearer when it's not advisable to use this
> helper because there are issues:
> - when connectors can be hotplugged (e.g. dp mst). For that case you need
>    to init the fbdev before you enabling hotplugging, but call
>    initial_config after you've enabled hotplugging. Otherwise there's a
>    posssible race window.

I have made a propose DOC change in patch 4.

> - when there's anything else that mus be done before we do the fbdev
>    registration, e.g. defio_init.

This happens in .fb_probe callback. See my reply to patch 5.

> So still not 100% sure this will help you all that much, but it's also not
> an unreasonable idea.
>
> Assuming we can agree on the ceveats and how do document them:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>> + *
>> + * See also: drm_fb_helper_initial_config()
>> + *
>> + * Returns:
>> + * Zero on success or negative error code on failure.
>> + */
>> +int drm_fb_helper_fbdev_setup(struct drm_device *dev,
>> +			      struct drm_fb_helper *fb_helper,
>> +			      const struct drm_fb_helper_funcs *funcs,
>> +			      unsigned int preferred_bpp,
>> +			      unsigned int max_conn_count)
>> +{
>> +	int ret;
>> +
>> +	if (!preferred_bpp)
>> +		preferred_bpp = dev->mode_config.preferred_depth;
>> +	if (!preferred_bpp)
>> +		preferred_bpp = 32;
>> +
>> +	if (!max_conn_count)
>> +		max_conn_count = dev->mode_config.num_connector;
>> +	if (!max_conn_count) {
>> +		DRM_DEV_ERROR(dev->dev, "No connectors\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	drm_fb_helper_prepare(dev, fb_helper, funcs);
>> +
>> +	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
>> +	if (ret < 0) {
>> +		DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
>> +	if (ret < 0) {
>> +		DRM_DEV_ERROR(dev->dev, "Failed to add connectors\n");
>> +		goto err_drm_fb_helper_fini;
>> +	}
>> +
>> +	if (!drm_drv_uses_atomic_modeset(dev))
>> +		drm_helper_disable_unused_functions(dev);
>> +
>> +	ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
>> +	if (ret < 0) {
>> +		DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration\n");
>> +		goto err_drm_fb_helper_fini;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_drm_fb_helper_fini:
>> +	drm_fb_helper_fini(fb_helper);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
>> +
>> +/**
>> + * drm_fb_helper_fbdev_teardown - Tear down fbdev emulation
>> + * @dev: DRM device
>> + *
>> + * This function unregisters fbdev if not already done and cleans up the
>> + * associated resources including the &drm_framebuffer.
>> + * The driver is responsible for freeing the &drm_fb_helper structure which is
>> + * stored in &drm_device->fb_helper. Do note that this pointer has been cleared
>> + * when this function returns.
>> + *
>> + * In order to support device removal/unplug while file handles are still open,
>> + * drm_fb_helper_unregister_fbi() should be called on device removal and
>> + * drm_fb_helper_fbdev_teardown() in the &drm_driver->release callback when
>> + * file handles are closed.
>> + */
>> +void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>> +{
>> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
>> +
>> +	if (!fb_helper)
>> +		return;
>> +
>> +	/* Unregister if it hasn't been done already */
>> +	if (fb_helper->fbdev && fb_helper->fbdev->dev)
>> +		drm_fb_helper_unregister_fbi(fb_helper);
>> +
>> +	drm_fb_helper_fini(fb_helper);
>> +
>> +	if (fb_helper->fb)
>> +		drm_framebuffer_remove(fb_helper->fb);
>> +}
>> +EXPORT_SYMBOL(drm_fb_helper_fbdev_teardown);
>> +
>>   /**
>>    * drm_fb_helper_lastclose - DRM driver lastclose helper for fbdev emulation
>>    * @dev: DRM device
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 1bd624579db7..aa78ac3b8ad0 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -320,6 +320,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>>   int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>>   				       struct drm_connector *connector);
>>   
>> +int drm_fb_helper_fbdev_setup(struct drm_device *dev,
>> +			      struct drm_fb_helper *fb_helper,
>> +			      const struct drm_fb_helper_funcs *funcs,
>> +			      unsigned int preferred_bpp,
>> +			      unsigned int max_conn_count);
>> +void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
>> +
>>   void drm_fb_helper_lastclose(struct drm_device *dev);
>>   void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>>   #else
>> @@ -525,6 +532,24 @@ drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>>   	return 0;
>>   }
>>   
>> +static inline int
>> +drm_fb_helper_fbdev_setup(struct drm_device *dev,
>> +			  struct drm_fb_helper *fb_helper,
>> +			  const struct drm_fb_helper_funcs *funcs,
>> +			  unsigned int preferred_bpp,
>> +			  unsigned int max_conn_count)
>> +{
>> +	/* So drivers can use it to free the struct */
>> +	dev->fb_helper = fb_helper;
>> +
>> +	return 0;
>> +}
>> +
>> +static inline void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>> +{
>> +	dev->fb_helper = NULL;
>> +}
>> +
>>   static inline void drm_fb_helper_lastclose(struct drm_device *dev)
>>   {
>>   }
>> -- 
>> 2.14.2
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Daniel Vetter]

On Fri, Dec 15, 2017 at 2:18 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 14.12.2017 21.25, skrev Daniel Vetter:
>>
>> On Thu, Dec 14, 2017 at 08:10:06PM +0100, Noralf Trønnes wrote:
>>>
>>> Add helper for initializing fbdev deferred I/O.
>>>
>>> The cleanup could have happened in drm_fb_helper_fini(), but that would
>>> have required me to set fb_info->fbdefio to NULL in a couple of drivers
>>> before they call _fini() to avoid double defio cleanup. The problem is
>>> that one of those is vboxvideo which lives in Greg's staging tree.
>>> So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
>>> but not that bad either.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/gpu/drm/drm_fb_helper.c | 53
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_fb_helper.h     |  6 +++++
>>>   2 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>> b/drivers/gpu/drm/drm_fb_helper.c
>>> index 14aa83579e76..d5eeed1c7749 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info
>>> *info,
>>>   }
>>>   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>>   +/**
>>> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>>> + * @fb_helper: driver-allocated fbdev helper
>>> + *
>>> + * This function allocates &fb_deferred_io, sets callback to
>>> + * drm_fb_helper_deferred_io(), delay to 50ms and calls
>>> fb_deferred_io_init().
>>> + * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>>> + *
>>> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is
>>> done
>>> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would
>>> thereby
>>> + * affect other instances of that &fb_ops.
>>
>> Do we need to call this before initial_config? Or after? Should be
>> documented imo.
>
>
> Indeed it should be:
>
>  * This function allocates &fb_deferred_io, sets callback to
>  * drm_fb_helper_deferred_io(), delay to 50ms and calls
> fb_deferred_io_init().
>  * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>  * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>
>> Also, did you look into just fixing fb_deferred_io_cleanup to no longer do
>> this? Changing function tables is definitely not cool (because that means
>> we can't put them into read-only memory and protect them from attackers -
>> function tables are a high-value target in the kernel, since usually easy
>> to trigger them).
>
>
> The fbdev operations isn't const:
>
> struct fb_info {
>     struct fb_ops *fbops;
> };
>
> Fixing that is a project of it's own as a quick grep revealed:
>
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_fillrect =
> cfb_fillrect;
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_copyarea =
> cfb_copyarea;
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_imageblit =
> cfb_imageblit;
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_fillrect =
> mb86290fb_fillrect;
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_copyarea =
> mb86290fb_copyarea;
> drivers/video/fbdev/mb862xx/mb862xxfb_accel.c: info->fbops->fb_imageblit =
> mb86290fb_imageblit;
>
> drivers/video/fbdev/uvesafb.c:          info->fbops->fb_blank = NULL;
> drivers/video/fbdev/uvesafb.c: info->fbops->fb_pan_display = NULL;
>
> drivers/video/fbdev/aty/atyfb_base.c: info->fbops->fb_sync = atyfb_sync;
> drivers/video/fbdev/aty/atyfb_base.c: info->fbops->fb_sync = NULL;
>
> drivers/video/fbdev/aty/mach64_cursor.c: info->fbops->fb_cursor =
> atyfb_cursor;
>
> drivers/video/fbdev/core/fb_defio.c:    info->fbops->fb_mmap =
> fb_deferred_io_mmap;
> drivers/video/fbdev/core/fb_defio.c:    info->fbops->fb_mmap = NULL;

Ah, it frobs the mmap callback with it's own.

> drivers/video/fbdev/vesafb.c: info->fbops->fb_pan_display = NULL;
>
> drivers/video/fbdev/udlfb.c:            info->fbops->fb_mmap =
> dlfb_ops_mmap;
>
> drivers/video/fbdev/smscufx.c:          info->fbops->fb_mmap = ufx_ops_mmap;
>
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_imageblit =
> nvidiafb_imageblit;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_fillrect =
> nvidiafb_fillrect;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_copyarea =
> nvidiafb_copyarea;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_sync = nvidiafb_sync;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_imageblit =
> cfb_imageblit;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_fillrect =
> cfb_fillrect;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_copyarea =
> cfb_copyarea;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_sync = NULL;
> drivers/video/fbdev/nvidia/nvidia.c: info->fbops->fb_cursor = NULL;

^^ this one we can probably delete, use tegra or nouveau instead.

> drivers/gpu/drm/udl/udl_fb.c:           info->fbops->fb_mmap = udl_fb_mmap;

Ugh.

> drivers/gpu/drm/drm_fb_cma_helper.c: fbi->fbops->fb_mmap =
> drm_fbdev_cma_deferred_io_mmap;

Yeah. Maybe a todo.rst entry would be good for this one here.
-Daniel

>
> Noralf.
>
>
>>
>>> + *
>>> + * Returns:
>>> + * 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>>> +{
>>> +       struct fb_info *info = fb_helper->fbdev;
>>> +       struct fb_deferred_io *fbdefio;
>>> +       struct fb_ops *fbops;
>>> +
>>> +       fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>>> +       fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>>> +       if (!fbdefio || !fbops) {
>>> +               kfree(fbdefio);
>>> +               kfree(fbops);
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       info->fbdefio = fbdefio;
>>> +       fbdefio->delay = msecs_to_jiffies(50);
>>> +       fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>> +
>>> +       *fbops = *info->fbops;
>>> +       info->fbops = fbops;
>>> +
>>> +       fb_deferred_io_init(info);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_fb_helper_defio_init);
>>> +
>>>   /**
>>>    * drm_fb_helper_sys_read - wrapper around fb_sys_read
>>>    * @info: fb_info struct pointer
>>> @@ -2743,6 +2785,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>>>    * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>>>    * function.
>>>    *
>>> + * Drivers that need fbdev deferred I/O should use
>>> drm_fb_helper_defio_init()
>>> + * to set it up.
>>
>> Not exactly sure why you put this here ... Maybe throw it into the
>> overview documentation, in a new paragraph that explains when you have a
>> non-NULL fb->dirty callback, then you most likely want to setup
>> defio_init. Except for the shmem fun. Explaining all that would be good
>> (maybe even put it under a "Framebuffer Flushing" heading).
>>
>> Otherwise looks all reasonable to me.
>>
>> Cheers, Daniel
>>
>>> + *
>>>    * See also: drm_fb_helper_initial_config()
>>>    *
>>>    * Returns:
>>> @@ -2818,6 +2863,7 @@ EXPORT_SYMBOL(drm_fb_helper_fbdev_setup);
>>>   void drm_fb_helper_fbdev_teardown(struct drm_device *dev)
>>>   {
>>>         struct drm_fb_helper *fb_helper = dev->fb_helper;
>>> +       struct fb_ops *fbops = NULL;
>>>         if (!fb_helper)
>>>                 return;
>>> @@ -2826,7 +2872,14 @@ void drm_fb_helper_fbdev_teardown(struct
>>> drm_device *dev)
>>>         if (fb_helper->fbdev && fb_helper->fbdev->dev)
>>>                 drm_fb_helper_unregister_fbi(fb_helper);
>>>   +     if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) {
>>> +               fb_deferred_io_cleanup(fb_helper->fbdev);
>>> +               kfree(fb_helper->fbdev->fbdefio);
>>> +               fbops = fb_helper->fbdev->fbops;
>>> +       }
>>> +
>>>         drm_fb_helper_fini(fb_helper);
>>> +       kfree(fbops);
>>>         if (fb_helper->fb)
>>>                 drm_framebuffer_remove(fb_helper->fb);
>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>> index aa78ac3b8ad0..b069433e7fc1 100644
>>> --- a/include/drm/drm_fb_helper.h
>>> +++ b/include/drm/drm_fb_helper.h
>>> @@ -276,6 +276,7 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper
>>> *fb_helper);
>>>     void drm_fb_helper_deferred_io(struct fb_info *info,
>>>                                struct list_head *pagelist);
>>> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>>>     ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user
>>> *buf,
>>>                                size_t count, loff_t *ppos);
>>> @@ -423,6 +424,11 @@ static inline void drm_fb_helper_deferred_io(struct
>>> fb_info *info,
>>>   {
>>>   }
>>>   +static inline int drm_fb_helper_defio_init(struct drm_fb_helper
>>> *fb_helper)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +
>>>   static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
>>>                                              char __user *buf, size_t
>>> count,
>>>                                              loff_t *ppos)
>>> --
>>> 2.14.2
>>>
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/fb-helper: Update DOC with new helpers

[Daniel Vetter]

On Fri, Dec 15, 2017 at 02:17:49PM +0100, Noralf Trønnes wrote:
> 
> Den 14.12.2017 20.10, skrev Noralf Trønnes:
> > Promote new helpers for dealing with fbdev emulation.
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >   drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 823fc8f50d85..14aa83579e76 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -66,19 +66,17 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >    * helper functions used by many drivers to implement the kernel mode setting
> >    * interfaces.
> >    *
> > - * Initialization is done as a four-step process with drm_fb_helper_prepare(),
> > - * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> > - * drm_fb_helper_initial_config(). Drivers with fancier requirements than the
> > - * default behaviour can override the third step with their own code.
> > - * Teardown is done with drm_fb_helper_fini() after the fbdev device is
> > - * unregisters using drm_fb_helper_unregister_fbi().
> > + * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> > + * down by calling drm_fb_helper_fbdev_teardown().
> >    *
> 
> How about this for documenting when _setup() can't be used:
> 
>  * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>  * down by calling drm_fb_helper_fbdev_teardown().
>  *
>  * Drivers that need to handle connector hotplugging (e.g. dp mst) can't use
>  * the setup helper and will need to do the whole four-step setup process
> with
>  * drm_fb_helper_prepare(), drm_fb_helper_init(),
>  * drm_fb_helper_single_add_all_connectors(), enable hotplugging and
>  * drm_fb_helper_initial_config() to avoid a possible race window.

lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on both this an
the other patch that inspired this update.
-Daniel

> 
> Noralf.
> 
> > - * At runtime drivers should restore the fbdev console by calling
> > - * drm_fb_helper_restore_fbdev_mode_unlocked() from their &drm_driver.lastclose
> > - * callback.  They should also notify the fb helper code from updates to the
> > - * output configuration by calling drm_fb_helper_hotplug_event(). For easier
> > - * integration with the output polling code in drm_crtc_helper.c the modeset
> > - * code provides a &drm_mode_config_funcs.output_poll_changed callback.
> > + * At runtime drivers should restore the fbdev console by using
> > + * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> > + * They should also notify the fb helper code from updates to the output
> > + * configuration by using drm_fb_helper_output_poll_changed() as their
> > + * &drm_mode_config_funcs.output_poll_changed callback.
> > + *
> > + * For suspend/resume consider using drm_mode_config_helper_suspend() and
> > + * drm_mode_config_helper_resume() which takes care of fbdev as well.
> >    *
> >    * All other functions exported by the fb helper library can be used to
> >    * implement the fbdev driver interface by the driver.
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Daniel Vetter]

On Fri, Dec 15, 2017 at 02:18:55PM +0100, Noralf Trønnes wrote:
> 
> Den 14.12.2017 21.25, skrev Daniel Vetter:
> > On Thu, Dec 14, 2017 at 08:10:06PM +0100, Noralf Trønnes wrote:
> > > Add helper for initializing fbdev deferred I/O.
> > > 
> > > The cleanup could have happened in drm_fb_helper_fini(), but that would
> > > have required me to set fb_info->fbdefio to NULL in a couple of drivers
> > > before they call _fini() to avoid double defio cleanup. The problem is
> > > that one of those is vboxvideo which lives in Greg's staging tree.
> > > So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
> > > but not that bad either.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++++++
> > >   include/drm/drm_fb_helper.h     |  6 +++++
> > >   2 files changed, 59 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 14aa83579e76..d5eeed1c7749 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> > > +/**
> > > + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> > > + * @fb_helper: driver-allocated fbdev helper
> > > + *
> > > + * This function allocates &fb_deferred_io, sets callback to
> > > + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> > > + * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> > > + *
> > > + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> > > + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> > > + * affect other instances of that &fb_ops.
> > Do we need to call this before initial_config? Or after? Should be
> > documented imo.
> 
> Indeed it should be:
> 
>  * This function allocates &fb_deferred_io, sets callback to
>  * drm_fb_helper_deferred_io(), delay to 50ms and calls
> fb_deferred_io_init().
>  * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>  * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.

Forgot this part, lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I think we got them all now?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()

[Noralf Trønnes]

Den 15.12.2017 16.43, skrev Daniel Vetter:
> On Fri, Dec 15, 2017 at 02:18:55PM +0100, Noralf Trønnes wrote:
>> Den 14.12.2017 21.25, skrev Daniel Vetter:
>>> On Thu, Dec 14, 2017 at 08:10:06PM +0100, Noralf Trønnes wrote:
>>>> Add helper for initializing fbdev deferred I/O.
>>>>
>>>> The cleanup could have happened in drm_fb_helper_fini(), but that would
>>>> have required me to set fb_info->fbdefio to NULL in a couple of drivers
>>>> before they call _fini() to avoid double defio cleanup. The problem is
>>>> that one of those is vboxvideo which lives in Greg's staging tree.
>>>> So I put the cleanup in drm_fb_helper_fbdev_teardown(), not perfect
>>>> but not that bad either.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>    drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_fb_helper.h     |  6 +++++
>>>>    2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 14aa83579e76..d5eeed1c7749 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -1022,6 +1022,48 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>    }
>>>>    EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>>> +/**
>>>> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>>>> + * @fb_helper: driver-allocated fbdev helper
>>>> + *
>>>> + * This function allocates &fb_deferred_io, sets callback to
>>>> + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>>>> + * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>>>> + *
>>>> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>>>> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>>>> + * affect other instances of that &fb_ops.
>>> Do we need to call this before initial_config? Or after? Should be
>>> documented imo.
>> Indeed it should be:
>>
>>   * This function allocates &fb_deferred_io, sets callback to
>>   * drm_fb_helper_deferred_io(), delay to 50ms and calls
>> fb_deferred_io_init().
>>   * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>>   * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> Forgot this part, lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I think we got them all now?

Indeed, thanks Daniel.

Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel