[PATCH 5/7] drm/fb-helper: Add drm_fb_helper_defio_init()
Noralf Trønnes
noralf at tronnes.org
Fri Dec 15 13:18:55 UTC 2017
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 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.
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
>>
More information about the dri-devel
mailing list