[PATCH v2 3/8] drm/fb-helper: Add simple init/fini functions
Daniel Vetter
daniel at ffwll.ch
Tue Oct 17 12:48:41 UTC 2017
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)?
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list