Re: [RFC v2 6/8] drm: Handle fbdev emulation in core

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
> Prepare for generic fbdev emulation by letting DRM core work directly
> with the fbdev compatibility layer. This is done by adding new fbdev
> helper vtable callbacks for restore, hotplug_event, unregister and
> release.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

No clue whether my idea is any good, but inspired by the bootsplash
discussion, and the prospect that we might get other in-kernel drm/kms
clients I'm wondering whether we should make this a bit more generic and
tie it to drm_file?

The idea would be to have a list of internal drm clients by putting all
the internal drm_files onto a list (same way we do with the userspace
ones). Then we'd just walk that list and call ->hotplug, ->unregister and
->release at the right places.

->restore would be a bit different, I guess for that we'd only call the
->restore callback of the very first kernel-internal client.

With that we could then add/remove kernel-internal drm clients like normal
userspace clients, which should make integration of emergency consoles,
boot splashes and whatever else real easy. And I think it would be a lot
cleaner than leaking fb_helper knowledge into the drm core, which feels a
rather backwards.

Otoh that feels a bit overengineered (but at least it shouldn't be a lot
more code really). If the list is too much work we could start with 1
drm_file * pointer for internal stuff (but would still need locking, so
list_head is probably easier).

Thoughts?
-Daniel

> ---
>  drivers/gpu/drm/drm_file.c         | 12 +++++++++++-
>  drivers/gpu/drm/drm_mode_config.c  | 10 ++++++++++
>  drivers/gpu/drm/drm_probe_helper.c |  4 ++++
>  include/drm/drm_fb_helper.h        | 33 +++++++++++++++++++++++++++++++++
>  4 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 400d44437e93..7ec09fb83135 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -35,6 +35,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_file.h>
>  #include <drm/drmP.h>
>  
> @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
>  
>  void drm_lastclose(struct drm_device * dev)
>  {
> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
> +	int ret;
> +
>  	DRM_DEBUG("\n");
>  
> -	if (dev->driver->lastclose)
> +	if (dev->driver->lastclose) {
>  		dev->driver->lastclose(dev);
> +	} else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
> +		ret = fb_helper->funcs->restore(fb_helper);
> +		if (ret)
> +			DRM_ERROR("Failed to restore fbdev: %d\n", ret);
> +	}
> +
>  	DRM_DEBUG("driver lastclose completed\n");
>  
>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index bc5c46306b3d..260eb1730244 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -21,6 +21,7 @@
>   */
>  
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_mode_config.h>
>  #include <drm/drmP.h>
>  
> @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
>  
>  void drm_modeset_unregister_all(struct drm_device *dev)
>  {
> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
> +
> +	if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
> +		fb_helper->funcs->unregister(fb_helper);
> +
>  	drm_connector_unregister_all(dev);
>  	drm_encoder_unregister_all(dev);
>  	drm_crtc_unregister_all(dev);
> @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
>   */
>  void drm_mode_config_cleanup(struct drm_device *dev)
>  {
> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	struct drm_crtc *crtc, *ct;
> @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	struct drm_property_blob *blob, *bt;
>  	struct drm_plane *plane, *plt;
>  
> +	if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
> +		fb_helper->funcs->release(fb_helper);
> +
>  	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
>  				 head) {
>  		encoder->funcs->destroy(encoder);
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 555fbe54d6e2..9d8b0ba54173 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
>   */
>  void drm_kms_helper_hotplug_event(struct drm_device *dev)
>  {
> +	struct drm_fb_helper *fb_helper = dev->fb_helper;
> +
>  	/* send a uevent + call fbdev */
>  	drm_sysfs_hotplug_event(dev);
>  	if (dev->mode_config.funcs->output_poll_changed)
>  		dev->mode_config.funcs->output_poll_changed(dev);
> +	else if (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
> +		fb_helper->funcs->hotplug_event(fb_helper);
>  }
>  EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
>  
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 16d8773b60e3..385f967c3552 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
>  			       struct drm_display_mode **modes,
>  			       struct drm_fb_offset *offsets,
>  			       bool *enabled, int width, int height);
> +
> +	/**
> +	 * @restore:
> +	 *
> +	 * Optional callback for restoring fbdev emulation.
> +	 * Called by drm_lastclose() if &drm_driver->lastclose is not set.
> +	 */
> +	int (*restore)(struct drm_fb_helper *fb_helper);
> +
> +	/**
> +	 * @hotplug_event:
> +	 *
> +	 * Optional callback for hotplug events.
> +	 * Called by drm_kms_helper_hotplug_event() if
> +	 * &drm_mode_config_funcs->output_poll_changed  is not set.
> +	 */
> +	int (*hotplug_event)(struct drm_fb_helper *fb_helper);
> +
> +	/**
> +	 * @unregister:
> +	 *
> +	 * Optional callback for unregistrering fbdev emulation.
> +	 * Called by drm_dev_unregister().
> +	 */
> +	void (*unregister)(struct drm_fb_helper *fb_helper);
> +
> +	/**
> +	 * @release:
> +	 *
> +	 * Optional callback for releasing fbdev emulation resources.
> +	 * Called by drm_mode_config_cleanup().
> +	 */
> +	void (*release)(struct drm_fb_helper *fb_helper);
>  };
>  
>  struct drm_fb_helper_connector {
> -- 
> 2.14.2
> 

-- 
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