Re: [PATCH 1/4] drm/fb-helper: Support deferred setup

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 03, 2016 at 06:11:16PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> FB helper code falls back to a 1024x768 mode if no outputs are connected
> or don't report back any modes upon initialization. This can be annoying
> because outputs that are added to FB helper later on can't be used with
> FB helper if they don't support a matching mode.
> 
> The fallback is in place because VGA connectors can happen to report an
> unknown connection status even when they are in fact connected.
> 
> Some drivers have custom solutions in place to defer FB helper setup
> until at least one output is connected. But the logic behind these
> solutions is always the same and there is nothing driver-specific about
> it, so a better alterative is to fix the FB helper core and add support
> for all drivers automatically.
> 
> This patch adds support for deferred FB helper setup. It checks all the
> connectors for their connection status, and if all of them report to be
> disconnected marks the FB helper as needing deferred setup. Whet setup
> is deferred, the FB helper core will automatically retry setup after a
> hotplug event, and it will keep trying until it succeeds.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h     | 21 +++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7c2eb75db60f..4f334f9cab28 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -442,6 +442,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return -ENODEV;
>  
> +	if (fb_helper->deferred_setup)
> +		return 0;
> +
>  	drm_modeset_lock_all(dev);
>  	ret = restore_fbdev_mode(fb_helper);
>  
> @@ -1398,6 +1401,23 @@ unlock:
>  }
>  EXPORT_SYMBOL(drm_fb_helper_pan_display);
>  
> +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
> +{
> +	bool connected = false;
> +	unsigned int i;
> +
> +	for (i = 0; i < helper->connector_count; i++) {
> +		struct drm_fb_helper_connector *fb = helper->connector_info[i];
> +
> +		if (fb->connector->status != connector_status_disconnected) {
> +			connected = true;
> +			break;
> +		}
> +	}
> +
> +	return connected;
> +}
> +
>  /*
>   * Allocates the backing storage and sets up the fbdev info structure through
>   * the ->fb_probe callback and then registers the fbdev and sets up the panic
> @@ -1499,6 +1519,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  			sizes.fb_height = min_t(u32, desired_mode->vdisplay + y, sizes.fb_height);
>  	}
>  
> +	/*
> +	 * If everything's disconnected, there's no use in attempting to set
> +	 * up fbdev.
> +	 */
> +	if (!drm_fb_helper_maybe_connected(fb_helper)) {
> +		DRM_INFO("No outputs connected, deferring setup\n");
> +		fb_helper->preferred_bpp = preferred_bpp;
> +		fb_helper->deferred_setup = true;
> +		return 0;
> +	}
> +
>  	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
>  		/* hmm everyone went away - assume VGA cable just fell out
>  		   and will come back later. */
> @@ -1539,6 +1570,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  
>  	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
>  
> +	fb_helper->deferred_setup = false;
>  	return 0;
>  }
>  
> @@ -2237,6 +2269,10 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	if (fb_helper->deferred_setup)
> +		return drm_fb_helper_initial_config(fb_helper,
> +						    fb_helper->preferred_bpp);

Logic looks all nice, but we need more locking. Extending our abuse of
dev->mode_config.mutex for fb_helper internals won't work since
initial_config already takes some locks at various times, we need a real
fb_helper->mutex now. We want that anyway, to be able to push a bunch of
the modeset locking out of the fbdev code, or at least down a lot.

To make it work we need to make it a top-level lock, surrounding all kms
locks I think. And minimally it needs to wrapt hotplug_event, restore_mode
and the single_add/remove_connector functions.
-Daniel

> +
>  	mutex_lock(&fb_helper->dev->mode_config.mutex);
>  	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>  		fb_helper->delayed_hotplug = true;
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5b4aa35026a3..25446690b2c1 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -214,6 +214,27 @@ struct drm_fb_helper {
>  	bool delayed_hotplug;
>  
>  	/**
> +	 * @deferred_setup:
> +	 *
> +	 * If no outputs are connected (disconnected or unknown) the FB helper
> +	 * code will defer setup until at least one of the outputs shows up.
> +	 * This field keeps track of the status so that setup can be retried
> +	 * at every hotplug event until it succeeds eventually.
> +	 */
> +	bool deferred_setup;
> +
> +	/**
> +	 * @preferred_bpp:
> +	 *
> +	 * Temporary storage for the driver's preferred BPP setting passed to
> +	 * FB helper initialization. This needs to be tracked so that deferred
> +	 * FB helper setup can pass this on.
> +	 *
> +	 * See also: @deferred_setup
> +	 */
> +	int preferred_bpp;
> +
> +	/**
>  	 * @atomic:
>  	 *
>  	 * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
> -- 
> 2.8.3
> 

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