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

Reply via email to