Re: [PATCH 9/9] drm/fb-helper: Finish the generic fbdev emulation

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 23, 2018 at 04:34:11PM +0200, Noralf Trønnes wrote:
> This adds a drm_fbdev_generic_setup() function that sets up generic
> fbdev emulation with client callbacks for lastclose, hotplug and
> remove/unregister.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 123 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h     |   7 +++
>  2 files changed, 130 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 444c2b4040ea..bc826457eb84 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -68,6 +68,9 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>   * helper functions used by many drivers to implement the kernel mode setting
>   * interfaces.
>   *
> + * Drivers that support a dumb buffer with a virtual address and mmap support,
> + * should try and use the generic fbdev emulation using drm_fbdev_generic_setup().
> + *
>   * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>   * down by calling drm_fb_helper_fbdev_teardown().
>   *
> @@ -3092,6 +3095,126 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_generic_probe);
>  
> +static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
> +	.fb_probe = drm_fb_helper_generic_probe,
> +};
> +
> +static int drm_fbdev_client_remove(struct drm_client_dev *client)
> +{
> +	struct drm_fb_helper *fb_helper = client->private;
> +
> +	/* Maybe setup was tried but failed */
> +	if (!fb_helper)
> +		return 0;
> +
> +	/* Maybe drm_fb_helper_fbdev_setup() hasn't run yet */
> +	if (!fb_helper->dev) {
> +		kfree(fb_helper);
> +		return 0;
> +	}
> +
> +	/* Maybe drm_fb_helper_generic_probe() hasn't run yet */
> +	if (!fb_helper->fbdev) {
> +		drm_fb_helper_fini(fb_helper);
> +		kfree(fb_helper);
> +		return 0;
> +	}
> +
> +	unregister_framebuffer(fb_helper->fbdev);

Hm, I'd expect nice problems with recursion and locking here, since we
clean up a lot of drm stuff from deeply within fbdev callbacks. I think
moving that cleanup code from your fb_destroy callback to here would look
a bit cleaner, and avoids the risk of going boom while holding the
console_lock. Which is always really, really nasty.

> +
> +	/*
> +	 * If userspace is closed the client is now freed by drm_fbdev_fb_destroy(),
> +	 * otherwise it will be freed on the last close.
> +	 * Return 1 to tell that freeing is taken care of.
> +	 */
> +	return 1;

Yeah that's definitely magic midlayer, we don't want the core to free
stuff for us and then pass around return values to tell it not to.

> +}
> +
> +static int drm_fbdev_client_lastclose(struct drm_client_dev *client)
> +{
> +	drm_fb_helper_restore_fbdev_mode_unlocked(client->private);
> +
> +	return 0;
> +}
> +
> +static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
> +{
> +	struct drm_fb_helper *fb_helper = client->private;
> +	struct drm_device *dev = client->dev;
> +	int ret;
> +
> +	if (dev->fb_helper)
> +		return drm_fb_helper_hotplug_event(dev->fb_helper);
> +
> +	if (!dev->mode_config.num_connector || !fb_helper)
> +		return 0;
> +
> +	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
> +					fb_helper->preferred_bpp, 0);
> +	if (ret) {
> +		/* Setup is tried only once */
> +		client->private = NULL;
> +		kfree(fb_helper);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_client_funcs drm_fbdev_client_funcs = {
> +	.name		= "fbdev",
> +	.remove		= drm_fbdev_client_remove,
> +	.lastclose	= drm_fbdev_client_lastclose,
> +	.hotplug	= drm_fbdev_client_hotplug,
> +};
> +
> +/**
> + * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
> + * @dev: DRM device
> + * @preferred_bpp: Preferred bits per pixel for the device.
> + *                 @dev->mode_config.preferred_depth is used if this is zero.
> + *
> + * This function sets up generic fbdev emulation for drivers that supports
> + * dumb buffers with a virtual address and that can be mmap'ed.
> + *
> + * Restore, hotplug events and teardown are all taken care of. Drivers that does
> + * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
> + * Simple drivers might use drm_mode_config_helper_suspend().
> + *
> + * This function is safe to call even when there are no connectors present.
> + * Setup will be retried on the next hotplug event.

I think we should spend a few more words on the limitations of the generic
fbdev support. Probably best to reference the generic_probe callback for
that (and make sure all the limitations, like defio vs. not vmalloced
buffers) are documented there.

lgtm otherwise. With the destroy story cleanup up and the kerneldoc
polished also has my

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> +{
> +	struct drm_fb_helper *fb_helper;
> +	struct drm_client_dev *client;
> +
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
> +	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> +	if (!fb_helper)
> +		return -ENOMEM;
> +
> +	client = drm_client_new(dev, &drm_fbdev_client_funcs);
> +	if (IS_ERR(client)) {
> +		kfree(fb_helper);
> +		return PTR_ERR(client);
> +	}
> +
> +	client->private = fb_helper;
> +	fb_helper->client = client;
> +	fb_helper->preferred_bpp = preferred_bpp;
> +
> +	drm_fbdev_client_hotplug(client);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_fbdev_generic_setup);
> +
>  /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
>   * but the module doesn't depend on any fb console symbols.  At least
>   * attempt to load fbcon to avoid leaving the system without a usable console.
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb38469a9502..884eabbc54d5 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -349,6 +349,7 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>  
>  int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>  				struct drm_fb_helper_surface_size *sizes);
> +int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
>  #else
>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>  					struct drm_fb_helper *helper,
> @@ -590,6 +591,12 @@ drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>  	return 0;
>  }
>  
> +static inline int
> +drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> +{
> +	return 0;
> +}
> +
>  #endif
>  
>  static inline int
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel