| Message ID | 20191025092759.13069-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: Documentation update for defio | expand |
Den 25.10.2019 11.27, skrev Thomas Zimmermann: > There are no users of drm_fb_helper_defio_init(), so we can remove > it. The documenation around defio support is a bit misleading and > should mention compatibility issues with SHMEM helpers. Clarify this. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- > include/drm/drm_fb_helper.h | 1 - > 2 files changed, 13 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index b75ae8555baf..8ebeccdeed23 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > * > * Drivers that support a dumb buffer with a virtual address and mmap support, > * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). > + * It will automatically set up deferred I/O if the driver requires a shadow > + * buffer. > * > - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it > - * down by calling drm_fb_helper_fbdev_teardown(). > + * For other drivers, 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 using > * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. > @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > * always run in process context since the fb_*() function could be running in > * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io > * callback it will also schedule dirty_work with the damage collected from the > - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup > - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). > + * mmap page writes. > + * > + * Deferred I/O is not compatible with SHMEM. Such drivers should request an > + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. > */ > > static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) > @@ -679,49 +684,6 @@ 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(). > - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. > - * 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); > - With this gone you can remove the defio cleanup part from drm_fb_helper_fbdev_teardown() as well. And I see that there's no users left of that function (the same with *_seup()). Would be nice if you just removed them. I made them before I embarked on the generic fbdev solution. I think it's better to try and make the generic emulation usable for "everyone" and avoid the need for drivers to have to do their own special stuff. Ofc speeding up the defio code to avoid the shadow buffering would aid in that as Daniel have talked about. Either way you choose, this patch is: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > /** > * drm_fb_helper_sys_read - wrapper around fb_sys_read > * @info: fb_info struct pointer > @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { > * > * Drivers that set the dirty callback on their framebuffer will get a shadow > * fbdev buffer that is blitted onto the real buffer. This is done in order to > - * make deferred I/O work with all kinds of buffers. > + * make deferred I/O work with all kinds of buffers. A shadow buffer can be > + * requested explicitly by setting struct drm_mode_config.prefer_shadow or > + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is > + * required to use generic fbdev emulation with SHMEM helpers. > * > * This function is safe to call even when there are no connectors present. > * Setup will be retried on the next hotplug event. > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index 8dcc012ccbc8..2338e9f94a03 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -235,7 +235,6 @@ 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); >
Hi Am 25.10.19 um 17:46 schrieb Noralf Trønnes: > > > Den 25.10.2019 11.27, skrev Thomas Zimmermann: >> There are no users of drm_fb_helper_defio_init(), so we can remove >> it. The documenation around defio support is a bit misleading and >> should mention compatibility issues with SHMEM helpers. Clarify this. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- >> include/drm/drm_fb_helper.h | 1 - >> 2 files changed, 13 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index b75ae8555baf..8ebeccdeed23 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >> * >> * Drivers that support a dumb buffer with a virtual address and mmap support, >> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). >> + * It will automatically set up deferred I/O if the driver requires a shadow >> + * buffer. >> * >> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it >> - * down by calling drm_fb_helper_fbdev_teardown(). >> + * For other drivers, 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 using >> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. >> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >> * always run in process context since the fb_*() function could be running in >> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io >> * callback it will also schedule dirty_work with the damage collected from the >> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup >> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). >> + * mmap page writes. >> + * >> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an >> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. >> */ >> >> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) >> @@ -679,49 +684,6 @@ 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(). >> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. >> - * 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); >> - > > With this gone you can remove the defio cleanup part from > drm_fb_helper_fbdev_teardown() as well. > > And I see that there's no users left of that function (the same with > *_seup()). Would be nice if you just removed them. I made them before I > embarked on the generic fbdev solution. I think it's better to try and > make the generic emulation usable for "everyone" and avoid the need for > drivers to have to do their own special stuff. The last user was vboxvideo, which I converted ~2 weeks ago. I haven't removed them yet, as there's a TODO item to convert drivers over to them. From a quick 'git grep': Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could probably be converted over to generic fbdev. That's hibmc (I have some untested patches for it), msm, omapdrm, tegra, and udl (currently being converted). Only nouveau and gma500 have some form of HW acceleration. The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are these strictly required, or can they be made available within generic fbdev? Best regards Thomas > code to avoid the shadow buffering would aid in that as Daniel have > talked about. > > Either way you choose, this patch is: > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > >> /** >> * drm_fb_helper_sys_read - wrapper around fb_sys_read >> * @info: fb_info struct pointer >> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { >> * >> * Drivers that set the dirty callback on their framebuffer will get a shadow >> * fbdev buffer that is blitted onto the real buffer. This is done in order to >> - * make deferred I/O work with all kinds of buffers. >> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be >> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or >> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is >> + * required to use generic fbdev emulation with SHMEM helpers. >> * >> * This function is safe to call even when there are no connectors present. >> * Setup will be retried on the next hotplug event. >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >> index 8dcc012ccbc8..2338e9f94a03 100644 >> --- a/include/drm/drm_fb_helper.h >> +++ b/include/drm/drm_fb_helper.h >> @@ -235,7 +235,6 @@ 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); >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 25.10.19 um 17:46 schrieb Noralf Trønnes: > > > > > > Den 25.10.2019 11.27, skrev Thomas Zimmermann: > >> There are no users of drm_fb_helper_defio_init(), so we can remove > >> it. The documenation around defio support is a bit misleading and > >> should mention compatibility issues with SHMEM helpers. Clarify this. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- > >> include/drm/drm_fb_helper.h | 1 - > >> 2 files changed, 13 insertions(+), 49 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >> index b75ae8555baf..8ebeccdeed23 100644 > >> --- a/drivers/gpu/drm/drm_fb_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_helper.c > >> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >> * > >> * Drivers that support a dumb buffer with a virtual address and mmap support, > >> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). > >> + * It will automatically set up deferred I/O if the driver requires a shadow > >> + * buffer. > >> * > >> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it > >> - * down by calling drm_fb_helper_fbdev_teardown(). > >> + * For other drivers, 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 using > >> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. > >> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >> * always run in process context since the fb_*() function could be running in > >> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io > >> * callback it will also schedule dirty_work with the damage collected from the > >> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup > >> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). > >> + * mmap page writes. > >> + * > >> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an > >> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. > >> */ > >> > >> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) > >> @@ -679,49 +684,6 @@ 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(). > >> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. > >> - * 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); > >> - > > > > With this gone you can remove the defio cleanup part from > > drm_fb_helper_fbdev_teardown() as well. > > > > And I see that there's no users left of that function (the same with > > *_seup()). Would be nice if you just removed them. I made them before I > > embarked on the generic fbdev solution. I think it's better to try and > > make the generic emulation usable for "everyone" and avoid the need for > > drivers to have to do their own special stuff. > > The last user was vboxvideo, which I converted ~2 weeks ago. I haven't > removed them yet, as there's a TODO item to convert drivers over to them. > > From a quick 'git grep': > > Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could > probably be converted over to generic fbdev. That's hibmc (I have some > untested patches for it), msm, omapdrm, tegra, and udl (currently being > converted). > > Only nouveau and gma500 have some form of HW acceleration. > > The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are > these strictly required, or can they be made available within generic fbdev? Take a pile of digging through a few fb horrors, but this boils down to: *sys* functions operate on an fb that works like normal system memory. *cfb* functions operate on void __iomem * instead. Which makes a huge difference on some architectures, but afaiui neither x86 nor arm care. On ppc it seems to make a difference sometimes. So for shmem we should use *sys* functions, for vram *cfb*. And that leads me to realizing that drm_gem_vram_vmap has the totally wrong type, it should be void __iomem *. There's this fancy is_iomem parameter for ttm_kmap_obj_virtual that should help you figure out the right type, but only nouveau bothers to implement this correctly. I'm honestly not sure whether we should care. -Daniel > > Best regards > Thomas > > > code to avoid the shadow buffering would aid in that as Daniel have > > talked about. > > > > Either way you choose, this patch is: > > > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > > > >> /** > >> * drm_fb_helper_sys_read - wrapper around fb_sys_read > >> * @info: fb_info struct pointer > >> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { > >> * > >> * Drivers that set the dirty callback on their framebuffer will get a shadow > >> * fbdev buffer that is blitted onto the real buffer. This is done in order to > >> - * make deferred I/O work with all kinds of buffers. > >> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be > >> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or > >> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is > >> + * required to use generic fbdev emulation with SHMEM helpers. > >> * > >> * This function is safe to call even when there are no connectors present. > >> * Setup will be retried on the next hotplug event. > >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > >> index 8dcc012ccbc8..2338e9f94a03 100644 > >> --- a/include/drm/drm_fb_helper.h > >> +++ b/include/drm/drm_fb_helper.h > >> @@ -235,7 +235,6 @@ 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); > >> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Hi Am 25.10.19 um 20:54 schrieb Daniel Vetter: > On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 25.10.19 um 17:46 schrieb Noralf Trønnes: >>> >>> >>> Den 25.10.2019 11.27, skrev Thomas Zimmermann: >>>> There are no users of drm_fb_helper_defio_init(), so we can remove >>>> it. The documenation around defio support is a bit misleading and >>>> should mention compatibility issues with SHMEM helpers. Clarify this. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- >>>> include/drm/drm_fb_helper.h | 1 - >>>> 2 files changed, 13 insertions(+), 49 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>>> index b75ae8555baf..8ebeccdeed23 100644 >>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>> * >>>> * Drivers that support a dumb buffer with a virtual address and mmap support, >>>> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). >>>> + * It will automatically set up deferred I/O if the driver requires a shadow >>>> + * buffer. >>>> * >>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it >>>> - * down by calling drm_fb_helper_fbdev_teardown(). >>>> + * For other drivers, 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 using >>>> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. >>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>> * always run in process context since the fb_*() function could be running in >>>> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io >>>> * callback it will also schedule dirty_work with the damage collected from the >>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup >>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). >>>> + * mmap page writes. >>>> + * >>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an >>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. >>>> */ >>>> >>>> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) >>>> @@ -679,49 +684,6 @@ 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(). >>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. >>>> - * 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); >>>> - >>> >>> With this gone you can remove the defio cleanup part from >>> drm_fb_helper_fbdev_teardown() as well. >>> >>> And I see that there's no users left of that function (the same with >>> *_seup()). Would be nice if you just removed them. I made them before I >>> embarked on the generic fbdev solution. I think it's better to try and >>> make the generic emulation usable for "everyone" and avoid the need for >>> drivers to have to do their own special stuff. >> >> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't >> removed them yet, as there's a TODO item to convert drivers over to them. >> >> From a quick 'git grep': >> >> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could >> probably be converted over to generic fbdev. That's hibmc (I have some >> untested patches for it), msm, omapdrm, tegra, and udl (currently being >> converted). >> >> Only nouveau and gma500 have some form of HW acceleration. >> >> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are >> these strictly required, or can they be made available within generic fbdev? > > Take a pile of digging through a few fb horrors, but this boils down to: > > *sys* functions operate on an fb that works like normal system memory. > > *cfb* functions operate on void __iomem * instead. Which makes a huge > difference on some architectures, but afaiui neither x86 nor arm care. > On ppc it seems to make a difference sometimes. > > So for shmem we should use *sys* functions, for vram *cfb*. > > And that leads me to realizing that drm_gem_vram_vmap has the totally > wrong type, it should be void __iomem *. There's this fancy is_iomem > parameter for ttm_kmap_obj_virtual that should help you figure out the > right type, but only nouveau bothers to implement this correctly. > > I'm honestly not sure whether we should care. I remember getting an eamil about this from some automated test system. I haven't had time to change it and it's apparently not urgent. If we really want to fix the problem, we'd have to propagage is_iomem through the DRM core; probably touching every vmap callback. OTOH this might not be a bad thing. With is_iomem available in the generic fbdev emulation, it could select between sys and cfb functions and support drivers with cfb-based fbdev as well. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> code to avoid the shadow buffering would aid in that as Daniel have >>> talked about. >>> >>> Either way you choose, this patch is: >>> >>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> >>> >>>> /** >>>> * drm_fb_helper_sys_read - wrapper around fb_sys_read >>>> * @info: fb_info struct pointer >>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { >>>> * >>>> * Drivers that set the dirty callback on their framebuffer will get a shadow >>>> * fbdev buffer that is blitted onto the real buffer. This is done in order to >>>> - * make deferred I/O work with all kinds of buffers. >>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be >>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or >>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is >>>> + * required to use generic fbdev emulation with SHMEM helpers. >>>> * >>>> * This function is safe to call even when there are no connectors present. >>>> * Setup will be retried on the next hotplug event. >>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >>>> index 8dcc012ccbc8..2338e9f94a03 100644 >>>> --- a/include/drm/drm_fb_helper.h >>>> +++ b/include/drm/drm_fb_helper.h >>>> @@ -235,7 +235,6 @@ 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); >>>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > >
On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote: > Hi > > Am 25.10.19 um 20:54 schrieb Daniel Vetter: > > On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi > >> > >> Am 25.10.19 um 17:46 schrieb Noralf Trønnes: > >>> > >>> > >>> Den 25.10.2019 11.27, skrev Thomas Zimmermann: > >>>> There are no users of drm_fb_helper_defio_init(), so we can remove > >>>> it. The documenation around defio support is a bit misleading and > >>>> should mention compatibility issues with SHMEM helpers. Clarify this. > >>>> > >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >>>> --- > >>>> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- > >>>> include/drm/drm_fb_helper.h | 1 - > >>>> 2 files changed, 13 insertions(+), 49 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >>>> index b75ae8555baf..8ebeccdeed23 100644 > >>>> --- a/drivers/gpu/drm/drm_fb_helper.c > >>>> +++ b/drivers/gpu/drm/drm_fb_helper.c > >>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >>>> * > >>>> * Drivers that support a dumb buffer with a virtual address and mmap support, > >>>> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). > >>>> + * It will automatically set up deferred I/O if the driver requires a shadow > >>>> + * buffer. > >>>> * > >>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it > >>>> - * down by calling drm_fb_helper_fbdev_teardown(). > >>>> + * For other drivers, 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 using > >>>> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. > >>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >>>> * always run in process context since the fb_*() function could be running in > >>>> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io > >>>> * callback it will also schedule dirty_work with the damage collected from the > >>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup > >>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). > >>>> + * mmap page writes. > >>>> + * > >>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an > >>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. > >>>> */ > >>>> > >>>> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) > >>>> @@ -679,49 +684,6 @@ 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(). > >>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. > >>>> - * 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); > >>>> - > >>> > >>> With this gone you can remove the defio cleanup part from > >>> drm_fb_helper_fbdev_teardown() as well. > >>> > >>> And I see that there's no users left of that function (the same with > >>> *_seup()). Would be nice if you just removed them. I made them before I > >>> embarked on the generic fbdev solution. I think it's better to try and > >>> make the generic emulation usable for "everyone" and avoid the need for > >>> drivers to have to do their own special stuff. > >> > >> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't > >> removed them yet, as there's a TODO item to convert drivers over to them. > >> > >> From a quick 'git grep': > >> > >> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could > >> probably be converted over to generic fbdev. That's hibmc (I have some > >> untested patches for it), msm, omapdrm, tegra, and udl (currently being > >> converted). > >> > >> Only nouveau and gma500 have some form of HW acceleration. > >> > >> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are > >> these strictly required, or can they be made available within generic fbdev? > > > > Take a pile of digging through a few fb horrors, but this boils down to: > > > > *sys* functions operate on an fb that works like normal system memory. > > > > *cfb* functions operate on void __iomem * instead. Which makes a huge > > difference on some architectures, but afaiui neither x86 nor arm care. > > On ppc it seems to make a difference sometimes. > > > > So for shmem we should use *sys* functions, for vram *cfb*. > > > > And that leads me to realizing that drm_gem_vram_vmap has the totally > > wrong type, it should be void __iomem *. There's this fancy is_iomem > > parameter for ttm_kmap_obj_virtual that should help you figure out the > > right type, but only nouveau bothers to implement this correctly. > > > > I'm honestly not sure whether we should care. > > I remember getting an eamil about this from some automated test system. > I haven't had time to change it and it's apparently not urgent. > > If we really want to fix the problem, we'd have to propagage is_iomem > through the DRM core; probably touching every vmap callback. OTOH this > might not be a bad thing. With is_iomem available in the generic fbdev > emulation, it could select between sys and cfb functions and support > drivers with cfb-based fbdev as well. is_iomem doesn't work, it's hack, since the change is in the function prototypes/signatures. So we'd need to make sure we have 2 sets of kernel mapping functions for everything, plus 2 sets of anything that accesses through the kernel. It's a huge task, and I'm really not sure we have any architecture we care about this ... -Daniel > > Best regards > Thomas > > > -Daniel > > > >> > >> Best regards > >> Thomas > >> > >>> code to avoid the shadow buffering would aid in that as Daniel have > >>> talked about. > >>> > >>> Either way you choose, this patch is: > >>> > >>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > >>> > >>>> /** > >>>> * drm_fb_helper_sys_read - wrapper around fb_sys_read > >>>> * @info: fb_info struct pointer > >>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { > >>>> * > >>>> * Drivers that set the dirty callback on their framebuffer will get a shadow > >>>> * fbdev buffer that is blitted onto the real buffer. This is done in order to > >>>> - * make deferred I/O work with all kinds of buffers. > >>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be > >>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or > >>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is > >>>> + * required to use generic fbdev emulation with SHMEM helpers. > >>>> * > >>>> * This function is safe to call even when there are no connectors present. > >>>> * Setup will be retried on the next hotplug event. > >>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > >>>> index 8dcc012ccbc8..2338e9f94a03 100644 > >>>> --- a/include/drm/drm_fb_helper.h > >>>> +++ b/include/drm/drm_fb_helper.h > >>>> @@ -235,7 +235,6 @@ 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); > >>>> > >>> _______________________________________________ > >>> dri-devel mailing list > >>> dri-devel@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>> > >> > >> -- > >> Thomas Zimmermann > >> Graphics Driver Developer > >> SUSE Software Solutions Germany GmbH > >> Maxfeldstr. 5, 90409 Nürnberg, Germany > >> (HRB 36809, AG Nürnberg) > >> Geschäftsführer: Felix Imendörffer > >> > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Hi Daniel Am 04.11.19 um 10:55 schrieb Daniel Vetter: > On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote: >> Hi >> >> Am 25.10.19 um 20:54 schrieb Daniel Vetter: >>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> >>>> Hi >>>> >>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes: >>>>> >>>>> >>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann: >>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove >>>>>> it. The documenation around defio support is a bit misleading and >>>>>> should mention compatibility issues with SHMEM helpers. Clarify this. >>>>>> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>> --- >>>>>> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- >>>>>> include/drm/drm_fb_helper.h | 1 - >>>>>> 2 files changed, 13 insertions(+), 49 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>>>>> index b75ae8555baf..8ebeccdeed23 100644 >>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>>>> * >>>>>> * Drivers that support a dumb buffer with a virtual address and mmap support, >>>>>> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). >>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow >>>>>> + * buffer. >>>>>> * >>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it >>>>>> - * down by calling drm_fb_helper_fbdev_teardown(). >>>>>> + * For other drivers, 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 using >>>>>> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. >>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>>>> * always run in process context since the fb_*() function could be running in >>>>>> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io >>>>>> * callback it will also schedule dirty_work with the damage collected from the >>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup >>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). >>>>>> + * mmap page writes. >>>>>> + * >>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an >>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. >>>>>> */ >>>>>> >>>>>> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) >>>>>> @@ -679,49 +684,6 @@ 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(). >>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. >>>>>> - * 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); >>>>>> - >>>>> >>>>> With this gone you can remove the defio cleanup part from >>>>> drm_fb_helper_fbdev_teardown() as well. >>>>> >>>>> And I see that there's no users left of that function (the same with >>>>> *_seup()). Would be nice if you just removed them. I made them before I >>>>> embarked on the generic fbdev solution. I think it's better to try and >>>>> make the generic emulation usable for "everyone" and avoid the need for >>>>> drivers to have to do their own special stuff. >>>> >>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't >>>> removed them yet, as there's a TODO item to convert drivers over to them. >>>> >>>> From a quick 'git grep': >>>> >>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could >>>> probably be converted over to generic fbdev. That's hibmc (I have some >>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being >>>> converted). >>>> >>>> Only nouveau and gma500 have some form of HW acceleration. >>>> >>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are >>>> these strictly required, or can they be made available within generic fbdev? >>> >>> Take a pile of digging through a few fb horrors, but this boils down to: >>> >>> *sys* functions operate on an fb that works like normal system memory. >>> >>> *cfb* functions operate on void __iomem * instead. Which makes a huge >>> difference on some architectures, but afaiui neither x86 nor arm care. >>> On ppc it seems to make a difference sometimes. >>> >>> So for shmem we should use *sys* functions, for vram *cfb*. >>> >>> And that leads me to realizing that drm_gem_vram_vmap has the totally >>> wrong type, it should be void __iomem *. There's this fancy is_iomem >>> parameter for ttm_kmap_obj_virtual that should help you figure out the >>> right type, but only nouveau bothers to implement this correctly. >>> >>> I'm honestly not sure whether we should care. >> >> I remember getting an eamil about this from some automated test system. >> I haven't had time to change it and it's apparently not urgent. >> >> If we really want to fix the problem, we'd have to propagage is_iomem >> through the DRM core; probably touching every vmap callback. OTOH this >> might not be a bad thing. With is_iomem available in the generic fbdev >> emulation, it could select between sys and cfb functions and support >> drivers with cfb-based fbdev as well. > > is_iomem doesn't work, it's hack, since the change is in the function > prototypes/signatures. So we'd need to make sure we have 2 sets of kernel > mapping functions for everything, plus 2 sets of anything that accesses > through the kernel. > > It's a huge task, and I'm really not sure we have any architecture we care > about this ... But the caller of vmap() doesn't know if it is I/O memory. It has to get this information via vmap() from the memory manager. Having two individual vmap() functions (and picking the correct one) would be quite a burden to the caller. But what's wrong with casting the returned address to void __iomem* if is_iomem is true? To get an idea of how well this works, I made a patchset to propagate is_iomem through all the vmap() interfaces in DRM. I found that most drivers' memory managers are SHMEM- or CMA-based and don't have to bother. The rest is based on TTM, which already returns the correct value for is_iomem. I didn't modify dma-buf interfaces and simply assumed 'false' here. The final patches modify the fbdev emulation to use either sys or cfb functions, depending on the value of is_iomem. Admittedly, I don't have the hardware where it makes a differences, but the change wasn't that hard either. I can probably post the patchset later this week for RFC. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> -Daniel >>> >>>> >>>> Best regards >>>> Thomas >>>> >>>>> code to avoid the shadow buffering would aid in that as Daniel have >>>>> talked about. >>>>> >>>>> Either way you choose, this patch is: >>>>> >>>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> >>>>> >>>>>> /** >>>>>> * drm_fb_helper_sys_read - wrapper around fb_sys_read >>>>>> * @info: fb_info struct pointer >>>>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { >>>>>> * >>>>>> * Drivers that set the dirty callback on their framebuffer will get a shadow >>>>>> * fbdev buffer that is blitted onto the real buffer. This is done in order to >>>>>> - * make deferred I/O work with all kinds of buffers. >>>>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be >>>>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or >>>>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is >>>>>> + * required to use generic fbdev emulation with SHMEM helpers. >>>>>> * >>>>>> * This function is safe to call even when there are no connectors present. >>>>>> * Setup will be retried on the next hotplug event. >>>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >>>>>> index 8dcc012ccbc8..2338e9f94a03 100644 >>>>>> --- a/include/drm/drm_fb_helper.h >>>>>> +++ b/include/drm/drm_fb_helper.h >>>>>> @@ -235,7 +235,6 @@ 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); >>>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>> >>>> >>>> -- >>>> Thomas Zimmermann >>>> Graphics Driver Developer >>>> SUSE Software Solutions Germany GmbH >>>> Maxfeldstr. 5, 90409 Nürnberg, Germany >>>> (HRB 36809, AG Nürnberg) >>>> Geschäftsführer: Felix Imendörffer >>>> >>> >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > > > >
On Mon, Nov 04, 2019 at 07:48:35PM +0100, Thomas Zimmermann wrote: > Hi Daniel > > Am 04.11.19 um 10:55 schrieb Daniel Vetter: > > On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote: > >> Hi > >> > >> Am 25.10.19 um 20:54 schrieb Daniel Vetter: > >>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >>>> > >>>> Hi > >>>> > >>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes: > >>>>> > >>>>> > >>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann: > >>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove > >>>>>> it. The documenation around defio support is a bit misleading and > >>>>>> should mention compatibility issues with SHMEM helpers. Clarify this. > >>>>>> > >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >>>>>> --- > >>>>>> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- > >>>>>> include/drm/drm_fb_helper.h | 1 - > >>>>>> 2 files changed, 13 insertions(+), 49 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >>>>>> index b75ae8555baf..8ebeccdeed23 100644 > >>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c > >>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c > >>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >>>>>> * > >>>>>> * Drivers that support a dumb buffer with a virtual address and mmap support, > >>>>>> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). > >>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow > >>>>>> + * buffer. > >>>>>> * > >>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it > >>>>>> - * down by calling drm_fb_helper_fbdev_teardown(). > >>>>>> + * For other drivers, 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 using > >>>>>> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. > >>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); > >>>>>> * always run in process context since the fb_*() function could be running in > >>>>>> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io > >>>>>> * callback it will also schedule dirty_work with the damage collected from the > >>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup > >>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). > >>>>>> + * mmap page writes. > >>>>>> + * > >>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an > >>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. > >>>>>> */ > >>>>>> > >>>>>> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) > >>>>>> @@ -679,49 +684,6 @@ 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(). > >>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. > >>>>>> - * 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); > >>>>>> - > >>>>> > >>>>> With this gone you can remove the defio cleanup part from > >>>>> drm_fb_helper_fbdev_teardown() as well. > >>>>> > >>>>> And I see that there's no users left of that function (the same with > >>>>> *_seup()). Would be nice if you just removed them. I made them before I > >>>>> embarked on the generic fbdev solution. I think it's better to try and > >>>>> make the generic emulation usable for "everyone" and avoid the need for > >>>>> drivers to have to do their own special stuff. > >>>> > >>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't > >>>> removed them yet, as there's a TODO item to convert drivers over to them. > >>>> > >>>> From a quick 'git grep': > >>>> > >>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could > >>>> probably be converted over to generic fbdev. That's hibmc (I have some > >>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being > >>>> converted). > >>>> > >>>> Only nouveau and gma500 have some form of HW acceleration. > >>>> > >>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are > >>>> these strictly required, or can they be made available within generic fbdev? > >>> > >>> Take a pile of digging through a few fb horrors, but this boils down to: > >>> > >>> *sys* functions operate on an fb that works like normal system memory. > >>> > >>> *cfb* functions operate on void __iomem * instead. Which makes a huge > >>> difference on some architectures, but afaiui neither x86 nor arm care. > >>> On ppc it seems to make a difference sometimes. > >>> > >>> So for shmem we should use *sys* functions, for vram *cfb*. > >>> > >>> And that leads me to realizing that drm_gem_vram_vmap has the totally > >>> wrong type, it should be void __iomem *. There's this fancy is_iomem > >>> parameter for ttm_kmap_obj_virtual that should help you figure out the > >>> right type, but only nouveau bothers to implement this correctly. > >>> > >>> I'm honestly not sure whether we should care. > >> > >> I remember getting an eamil about this from some automated test system. > >> I haven't had time to change it and it's apparently not urgent. > >> > >> If we really want to fix the problem, we'd have to propagage is_iomem > >> through the DRM core; probably touching every vmap callback. OTOH this > >> might not be a bad thing. With is_iomem available in the generic fbdev > >> emulation, it could select between sys and cfb functions and support > >> drivers with cfb-based fbdev as well. > > > > is_iomem doesn't work, it's hack, since the change is in the function > > prototypes/signatures. So we'd need to make sure we have 2 sets of kernel > > mapping functions for everything, plus 2 sets of anything that accesses > > through the kernel. > > > > It's a huge task, and I'm really not sure we have any architecture we care > > about this ... > > But the caller of vmap() doesn't know if it is I/O memory. It has to get > this information via vmap() from the memory manager. Having two > individual vmap() functions (and picking the correct one) would be quite > a burden to the caller. > > But what's wrong with casting the returned address to void __iomem* if > is_iomem is true? > > To get an idea of how well this works, I made a patchset to propagate > is_iomem through all the vmap() interfaces in DRM. I found that most > drivers' memory managers are SHMEM- or CMA-based and don't have to > bother. The rest is based on TTM, which already returns the correct > value for is_iomem. I didn't modify dma-buf interfaces and simply > assumed 'false' here. > > The final patches modify the fbdev emulation to use either sys or cfb > functions, depending on the value of is_iomem. Admittedly, I don't have > the hardware where it makes a differences, but the change wasn't that > hard either. > > I can probably post the patchset later this week for RFC. The big reason behind __iomem is that you can use sparse to make sure you got it right. With is_iomem and the explicit cast, you'll lose that information. Which means no one will get it right (viz entire current drm except nouveau). That's why I think the 2 paths would be a lot nicer, and callers would need to first call the system memory mmap, then the iomem mmap, until they have a non-NULL pointer. Since they need the duplicated code anyway. Other option would be we do an entire new pointer like the below: struct opaque_buffer_ptr { union { void * smem; void * __iomem iomem ; }; bool is_iomem; }; And then an entire new set of functions that deals in this new primitive. But unloading the is_iomem explicitly on drivers, expecting them to get it right without the help or sparse, seems futile. Also, all of this is _huuuuuuuuge_ amounts of work, and I'm still not sure where we need it. -Daniel
Hi Am 05.11.19 um 10:30 schrieb Daniel Vetter: > On Mon, Nov 04, 2019 at 07:48:35PM +0100, Thomas Zimmermann wrote: >> Hi Daniel >> >> Am 04.11.19 um 10:55 schrieb Daniel Vetter: >>> On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote: >>>> Hi >>>> >>>> Am 25.10.19 um 20:54 schrieb Daniel Vetter: >>>>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>>>> >>>>>> Hi >>>>>> >>>>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes: >>>>>>> >>>>>>> >>>>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann: >>>>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove >>>>>>>> it. The documenation around defio support is a bit misleading and >>>>>>>> should mention compatibility issues with SHMEM helpers. Clarify this. >>>>>>>> >>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- >>>>>>>> include/drm/drm_fb_helper.h | 1 - >>>>>>>> 2 files changed, 13 insertions(+), 49 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>>>>>>> index b75ae8555baf..8ebeccdeed23 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>>>>>> * >>>>>>>> * Drivers that support a dumb buffer with a virtual address and mmap support, >>>>>>>> * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). >>>>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow >>>>>>>> + * buffer. >>>>>>>> * >>>>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it >>>>>>>> - * down by calling drm_fb_helper_fbdev_teardown(). >>>>>>>> + * For other drivers, 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 using >>>>>>>> * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. >>>>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); >>>>>>>> * always run in process context since the fb_*() function could be running in >>>>>>>> * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io >>>>>>>> * callback it will also schedule dirty_work with the damage collected from the >>>>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup >>>>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). >>>>>>>> + * mmap page writes. >>>>>>>> + * >>>>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an >>>>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. >>>>>>>> */ >>>>>>>> >>>>>>>> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) >>>>>>>> @@ -679,49 +684,6 @@ 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(). >>>>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. >>>>>>>> - * 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); >>>>>>>> - >>>>>>> >>>>>>> With this gone you can remove the defio cleanup part from >>>>>>> drm_fb_helper_fbdev_teardown() as well. >>>>>>> >>>>>>> And I see that there's no users left of that function (the same with >>>>>>> *_seup()). Would be nice if you just removed them. I made them before I >>>>>>> embarked on the generic fbdev solution. I think it's better to try and >>>>>>> make the generic emulation usable for "everyone" and avoid the need for >>>>>>> drivers to have to do their own special stuff. >>>>>> >>>>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't >>>>>> removed them yet, as there's a TODO item to convert drivers over to them. >>>>>> >>>>>> From a quick 'git grep': >>>>>> >>>>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could >>>>>> probably be converted over to generic fbdev. That's hibmc (I have some >>>>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being >>>>>> converted). >>>>>> >>>>>> Only nouveau and gma500 have some form of HW acceleration. >>>>>> >>>>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are >>>>>> these strictly required, or can they be made available within generic fbdev? >>>>> >>>>> Take a pile of digging through a few fb horrors, but this boils down to: >>>>> >>>>> *sys* functions operate on an fb that works like normal system memory. >>>>> >>>>> *cfb* functions operate on void __iomem * instead. Which makes a huge >>>>> difference on some architectures, but afaiui neither x86 nor arm care. >>>>> On ppc it seems to make a difference sometimes. >>>>> >>>>> So for shmem we should use *sys* functions, for vram *cfb*. >>>>> >>>>> And that leads me to realizing that drm_gem_vram_vmap has the totally >>>>> wrong type, it should be void __iomem *. There's this fancy is_iomem >>>>> parameter for ttm_kmap_obj_virtual that should help you figure out the >>>>> right type, but only nouveau bothers to implement this correctly. >>>>> >>>>> I'm honestly not sure whether we should care. >>>> >>>> I remember getting an eamil about this from some automated test system. >>>> I haven't had time to change it and it's apparently not urgent. >>>> >>>> If we really want to fix the problem, we'd have to propagage is_iomem >>>> through the DRM core; probably touching every vmap callback. OTOH this >>>> might not be a bad thing. With is_iomem available in the generic fbdev >>>> emulation, it could select between sys and cfb functions and support >>>> drivers with cfb-based fbdev as well. >>> >>> is_iomem doesn't work, it's hack, since the change is in the function >>> prototypes/signatures. So we'd need to make sure we have 2 sets of kernel >>> mapping functions for everything, plus 2 sets of anything that accesses >>> through the kernel. >>> >>> It's a huge task, and I'm really not sure we have any architecture we care >>> about this ... >> >> But the caller of vmap() doesn't know if it is I/O memory. It has to get >> this information via vmap() from the memory manager. Having two >> individual vmap() functions (and picking the correct one) would be quite >> a burden to the caller. >> >> But what's wrong with casting the returned address to void __iomem* if >> is_iomem is true? >> >> To get an idea of how well this works, I made a patchset to propagate >> is_iomem through all the vmap() interfaces in DRM. I found that most >> drivers' memory managers are SHMEM- or CMA-based and don't have to >> bother. The rest is based on TTM, which already returns the correct >> value for is_iomem. I didn't modify dma-buf interfaces and simply >> assumed 'false' here. >> >> The final patches modify the fbdev emulation to use either sys or cfb >> functions, depending on the value of is_iomem. Admittedly, I don't have >> the hardware where it makes a differences, but the change wasn't that >> hard either. >> >> I can probably post the patchset later this week for RFC. > > The big reason behind __iomem is that you can use sparse to make sure you > got it right. With is_iomem and the explicit cast, you'll lose that > information. Which means no one will get it right (viz entire current drm > except nouveau). > > That's why I think the 2 paths would be a lot nicer, and callers would > need to first call the system memory mmap, then the iomem mmap, until they > have a non-NULL pointer. Since they need the duplicated code anyway. > > Other option would be we do an entire new pointer like the below: > > struct opaque_buffer_ptr { > union { void * smem; void * __iomem iomem ; }; > bool is_iomem; > }; > > And then an entire new set of functions that deals in this new primitive. > > But unloading the is_iomem explicitly on drivers, expecting them to get it > right without the help or sparse, seems futile. > > Also, all of this is _huuuuuuuuge_ amounts of work, and I'm still not sure > where we need it. I'm telling you that I have the patches written, and you say it's too much work? That makes no sense. :) Let my post the patches that I have and continue discussing from there. Best regards Thomas > -Daniel >
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index b75ae8555baf..8ebeccdeed23 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); * * Drivers that support a dumb buffer with a virtual address and mmap support, * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). + * It will automatically set up deferred I/O if the driver requires a shadow + * buffer. * - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it - * down by calling drm_fb_helper_fbdev_teardown(). + * For other drivers, 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 using * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); * always run in process context since the fb_*() function could be running in * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io * callback it will also schedule dirty_work with the damage collected from the - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()). + * mmap page writes. + * + * Deferred I/O is not compatible with SHMEM. Such drivers should request an + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. */ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) @@ -679,49 +684,6 @@ 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(). - * It should be called from the &drm_fb_helper_funcs->fb_probe callback. - * 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 @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { * * Drivers that set the dirty callback on their framebuffer will get a shadow * fbdev buffer that is blitted onto the real buffer. This is done in order to - * make deferred I/O work with all kinds of buffers. + * make deferred I/O work with all kinds of buffers. A shadow buffer can be + * requested explicitly by setting struct drm_mode_config.prefer_shadow or + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is + * required to use generic fbdev emulation with SHMEM helpers. * * This function is safe to call even when there are no connectors present. * Setup will be retried on the next hotplug event. diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 8dcc012ccbc8..2338e9f94a03 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -235,7 +235,6 @@ 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);
There are no users of drm_fb_helper_defio_init(), so we can remove it. The documenation around defio support is a bit misleading and should mention compatibility issues with SHMEM helpers. Clarify this. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_fb_helper.c | 61 +++++++-------------------------- include/drm/drm_fb_helper.h | 1 - 2 files changed, 13 insertions(+), 49 deletions(-)