[PATCH v2 3/8] drm/fb-helper: Add simple init/fini functions
Noralf Trønnes
noralf at tronnes.org
Wed Oct 18 19:43:41 UTC 2017
Den 17.10.2017 14.48, skrev Daniel Vetter:
> On Sun, Oct 15, 2017 at 06:30:37PM +0200, Noralf Trønnes wrote:
>> This adds some simple init/fini helpers for drivers that don't need
>> anything special in their fbdev emulation setup.
>>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> So I know the saying goes that any design problem in IT can be solved by
> adding another layer, but layers have costs. I still feel like this here
> just papers over some of the evolved design issues in our fbdev helper
> setup/teardown logic.
>
> Do we _really_ need all of them, can't we fold at least some of this
> together?
>
> E.g. the split between initial_config and single_add_all connectors seems
> a bit funny. Same with prepare and init. And maybe we should just push the
> default bpp handling down into the helpers too (no point reinventing that
> wheel everywhere)?
Ok, I think I'll bail out of this one, at least for now. It will be too
much work and I need to focus on moving tinydrm away from the cma helper.
For those interested, here's a compilation of drm_fb_helper init/fini
use in all drivers:
https://gist.github.com/notro/d7fc2809736cdddb68a644b589a86ee1
I think I'll split out a patchset off of this one and do that first:
1. Add drm_device->drm_fb_helper_private
Set it in drm_fb_helper_init()
Clear it in drm_fb_helper_fini()
2. drm/fb-helper: Add .last_close and .output_poll_changed helpers
n. Convert drivers to use those helpers
Noralf.
>> ---
>> drivers/gpu/drm/drm_fb_helper.c | 163 ++++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_fb_helper.h | 22 ++++++
>> 2 files changed, 185 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 954cdd48de92..166535da9a9b 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -105,6 +105,127 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>> * mmap page writes.
>> */
>>
>> +/**
>> + * drm_fb_helper_simple_init - Simple fbdev emulation setup
>> + * @dev: DRM device
>> + * @funcs: Pointer to structure of functions associated with this helper
>> + * @bpp: Bits per pixel value to use for the framebuffer configuration.
>> + * @dev->mode_config.preferred_depth is used if this is zero.
>> + * @max_conn: Max connector count. @dev->mode_config.num_connector is used if
>> + * this is zero.
>> + *
>> + * Simple fbdev emulation initialization. This function allocates a
>> + * &drm_fb_helper structure attaches it to &drm_device->fbdev and calls:
>> + * drm_fb_helper_prepare(), drm_fb_helper_init(),
>> + * drm_fb_helper_single_add_all_connectors() and
>> + * drm_fb_helper_initial_config().
>> + *
>> + * If fbdev emulation is disabled, this function does nothing. In that case
>> + * &drm_device->fbdev will be NULL. All drm_fb_helper entry function can handle
>> + * a NULL argument.
>> + *
>> + * fbdev deferred I/O users should use the drm_fb_helper_defio_init() function
>> + * in their &drm_fb_helper_funcs.fb_probe callback.
>> + *
>> + * Returns:
>> + * Zero on success or if fbdev emulation is disabled, negative error code on
>> + * failure.
>> + */
>> +int drm_fb_helper_simple_init(struct drm_device *dev,
>> + const struct drm_fb_helper_funcs *funcs,
>> + unsigned int bpp, int max_conn)
>> +{
>> + struct drm_fb_helper *fb_helper;
>> + int ret;
>> +
>> + if (!drm_fbdev_emulation)
>> + return 0;
>> +
>> + if (!bpp)
>> + bpp = dev->mode_config.preferred_depth;
>> +
>> + if (!max_conn)
>> + max_conn = dev->mode_config.num_connector;
>> +
>> + fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>> + if (!fb_helper)
>> + return -ENOMEM;
>> +
>> + drm_fb_helper_prepare(dev, fb_helper, funcs);
>> +
>> + ret = drm_fb_helper_init(dev, fb_helper, max_conn);
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
>> + goto err_drm_fb_helper_free;
>> + }
>> +
>> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + ret = drm_fb_helper_initial_config(fb_helper, bpp);
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + dev->fbdev = fb_helper;
> This definitely needs to be in one of the above functions so that you
> don't force other drivers to either do this themselves or use this new
> helper to be able to benefit from the new convenience functions you've
> just added.
>
> I'd set it in prepare, that's the first point where we can set it.
>
>> +
>> + return 0;
>> +
>> +err_drm_fb_helper_fini:
>> + drm_fb_helper_fini(fb_helper);
>> +err_drm_fb_helper_free:
>> + kfree(fb_helper);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
>> +
>> +/**
>> + * drm_fb_helper_simple_fini - Simple fbdev cleanup
>> + * @dev: DRM device
>> + *
>> + * Simple fbdev emulation cleanup. This function unregisters fbdev if it is not
>> + * done, cleans up deferred IO if necessary, removes framebuffer, finalizes
>> + * @fb_helper and frees the structure.
>> + */
>> +void drm_fb_helper_simple_fini(struct drm_device *dev)
> Similar here on the cleanup side, can't we simplify this a lot for
> everyone?
>
>> +{
>> + struct drm_fb_helper *fb_helper = dev->fbdev;
>> + struct fb_info *info;
>> + struct fb_ops *fbops;
>> +
>> + if (!fb_helper)
>> + return;
>> +
>> + dev->fbdev = NULL;
>> + info = fb_helper->fbdev;
>> +
>> + /* Make sure it hasn't been unregistered already */
>> + if (info && info->dev)
>> + drm_fb_helper_unregister_fbi(fb_helper);
>> +
>> + if (info && info->fbdefio) {
>> + fb_deferred_io_cleanup(info);
>> + kfree(info->fbdefio);
>> + fbops = info->fbops;
>> + }
>> +
>> + drm_fb_helper_fini(fb_helper);
>> +
>> + if (info && info->fbdefio)
>> + kfree(fbops);
>> +
>> + if (fb_helper->fb)
>> + drm_framebuffer_remove(fb_helper->fb);
>> +
>> + kfree(fb_helper);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
>> +
>> #define drm_fb_helper_for_each_connector(fbh, i__) \
>> for (({ lockdep_assert_held(&(fbh)->lock); }), \
>> i__ = 0; i__ < (fbh)->connector_count; i__++)
>> @@ -954,6 +1075,48 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>> }
>> EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
>>
>> +/**
>> + * 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().
>> + *
>> + * 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. This copy is freed in
>> + * drm_fb_helper_simple_fini().
>> + *
>> + * 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);
> Probably good as a separate patch + using it in a bunch of drivers to show
> that it's the right kind of helper that's useful in a few places.
>
> Cheers, Daniel
>
>> +
>> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>> u32 width, u32 height)
>> {
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 33fe95927742..6503f4c3e3ef 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -242,6 +242,10 @@ struct drm_fb_helper {
>> .fb_ioctl = drm_fb_helper_ioctl
>>
>> #ifdef CONFIG_DRM_FBDEV_EMULATION
>> +int drm_fb_helper_simple_init(struct drm_device *dev,
>> + const struct drm_fb_helper_funcs *funcs,
>> + unsigned int bpp, int max_conn);
>> +void drm_fb_helper_simple_fini(struct drm_device *dev);
>> void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>> const struct drm_fb_helper_funcs *funcs);
>> int drm_fb_helper_init(struct drm_device *dev,
>> @@ -265,6 +269,7 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>>
>> void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>
>> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>> void drm_fb_helper_deferred_io(struct fb_info *info,
>> struct list_head *pagelist);
>>
>> @@ -311,6 +316,18 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>> int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>> struct drm_connector *connector);
>> #else
>> +static inline int
>> +drm_fb_helper_simple_init(struct drm_device *dev,
>> + const struct drm_fb_helper_funcs *funcs,
>> + unsigned int bpp, int max_conn)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void drm_fb_helper_simple_fini(struct drm_device *dev)
>> +{
>> +}
>> +
>> static inline void drm_fb_helper_prepare(struct drm_device *dev,
>> struct drm_fb_helper *helper,
>> const struct drm_fb_helper_funcs *funcs)
>> @@ -393,6 +410,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>> {
>> }
>>
>> +static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> static inline void drm_fb_helper_deferred_io(struct fb_info *info,
>> struct list_head *pagelist)
>> {
>> --
>> 2.14.2
>>
More information about the dri-devel
mailing list