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