[PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs
Thomas Zimmermann
tzimmermann at suse.de
Mon Oct 28 08:13:47 UTC 2019
Hi
Am 25.10.19 um 20:54 schrieb Daniel Vetter:
> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann at 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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191028/5febe0aa/attachment.sig>
More information about the dri-devel
mailing list