On Fri, Dec 15, 2017 at 2:18 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
