Re: [RFC v4 12/25] drm/i915: Add drm_driver->initial_client_display callback

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Apr 14, 2018 at 01:53:05PM +0200, Noralf Trønnes wrote:
> As part of moving the modesetting code out of drm_fb_helper and into
> drm_client, the drm_fb_helper_funcs->initial_config callback needs to go.
> Replace it with a drm_driver->initial_client_display callback that can
> work for all in-kernel clients.
> 
> TODO:
> - Add a patch that moves the function out of intel_fbdev.c since it's not
>   fbdev specific anymore.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

So the reason we originally added this callback for i915 fast boot was
that there wasn't any atomic around yet. And it was all an experiment to
figure out how to best go about designing fastboot.

But now we have fbdev, and fastboot design is also pretty clear:

1. driver loads
2. driver reads out current hw state, reconstructs a full atomic state for
everything and stuffs it into connector/crtc/plane->state pointers.
3. fbdev and any other client read out current state (with some caveats)
and just take it over.

What non-fastboot drivers do:
1. drivers load
2. reset both hw and sw state to everything off.

Now the intel_fb_initial_config is all generic code really, and it will
neatly fall back to the default config if everything is off. This means we
could:
1. Move the intel_fb_initial_config into the fbdev helpers.
2. Nuke the ->initial_config callback.

And pronto! every driver which implements hw state readout will get fbdev
fastboot for free.

And since you've already rewritting the intel code to use drm_client, it's
practically done already. Just need to s/intel_/drm_fbdev_helper_ or
something like that :-)
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c    |  19 +++++--
>  drivers/gpu/drm/i915/i915_drv.c    |   1 +
>  drivers/gpu/drm/i915/intel_drv.h   |  11 ++++
>  drivers/gpu/drm/i915/intel_fbdev.c | 113 ++++++++++++++++++-------------------
>  include/drm/drm_drv.h              |  21 +++++++
>  5 files changed, 104 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b992f59dad30..5407bf6dc8c0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2103,6 +2103,20 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	/* prevent concurrent modification of connector_count by hotplug */
>  	lockdep_assert_held(&fb_helper->lock);
>  
> +	mutex_lock(&dev->mode_config.mutex);
> +	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> +		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> +
> +	if (dev->driver->initial_client_display) {
> +		display = dev->driver->initial_client_display(dev, width, height);
> +		if (display) {
> +			drm_client_display_free(fb_helper->display);
> +			fb_helper->display = display;
> +			mutex_unlock(&dev->mode_config.mutex);
> +			return;
> +		}
> +	}
> +
>  	crtcs = kcalloc(fb_helper->connector_count,
>  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
>  	modes = kcalloc(fb_helper->connector_count,
> @@ -2120,9 +2134,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	if (IS_ERR(display))
>  		goto out;
>  
> -	mutex_lock(&fb_helper->dev->mode_config.mutex);
> -	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> -		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>  	drm_enable_connectors(fb_helper, enabled);
>  
>  	if (!(fb_helper->funcs->initial_config &&
> @@ -2144,7 +2155,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  
>  		drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
>  	}
> -	mutex_unlock(&fb_helper->dev->mode_config.mutex);
>  
>  	/* need to set the modesets up here for use later */
>  	/* fill out the connector<->crtc mappings into the modesets */
> @@ -2182,6 +2192,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	drm_client_display_free(fb_helper->display);
>  	fb_helper->display = display;
>  out:
> +	mutex_unlock(&dev->mode_config.mutex);
>  	kfree(crtcs);
>  	kfree(modes);
>  	kfree(offsets);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 07c07d55398b..b746c0cbaa4b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2857,6 +2857,7 @@ static struct drm_driver driver = {
>  
>  	.dumb_create = i915_gem_dumb_create,
>  	.dumb_map_offset = i915_gem_mmap_gtt,
> +	.initial_client_display = i915_initial_client_display,
>  	.ioctls = i915_ioctls,
>  	.num_ioctls = ARRAY_SIZE(i915_ioctls),
>  	.fops = &i915_driver_fops,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d4368589b355..f77f510617c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1720,6 +1720,9 @@ extern void intel_fbdev_fini(struct drm_i915_private *dev_priv);
>  extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
>  extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
>  extern void intel_fbdev_restore_mode(struct drm_device *dev);
> +struct drm_client_display *
> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> +			    unsigned int height);
>  #else
>  static inline int intel_fbdev_init(struct drm_device *dev)
>  {
> @@ -1749,6 +1752,14 @@ static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
>  static inline void intel_fbdev_restore_mode(struct drm_device *dev)
>  {
>  }
> +
> +static inline struct drm_client_display *
> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> +			    unsigned int height)
> +{
> +	return NULL;
> +}
> +
>  #endif
>  
>  /* intel_fbc.c */
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index a4ab8575a72e..b7f44c9475a8 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -38,6 +38,7 @@
>  #include <linux/vga_switcheroo.h>
>  
>  #include <drm/drmP.h>
> +#include <drm/drm_client.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include "intel_drv.h"
> @@ -287,18 +288,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	return ret;
>  }
>  
> -static struct drm_fb_helper_crtc *
> -intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> -{
> -	int i;
> -
> -	for (i = 0; i < fb_helper->crtc_count; i++)
> -		if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> -			return &fb_helper->crtc_info[i];
> -
> -	return NULL;
> -}
> -
>  /*
>   * Try to read the BIOS display configuration and use it for the initial
>   * fb configuration.
> @@ -326,44 +315,48 @@ intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
>   * is in VGA mode we need to recalculate watermarks and set a new high-res
>   * framebuffer anyway.
>   */
> -static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> -				    struct drm_fb_helper_crtc **crtcs,
> -				    struct drm_display_mode **modes,
> -				    struct drm_fb_offset *offsets,
> -				    bool *enabled, int width, int height)
> +struct drm_client_display *
> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> +			    unsigned int height)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long conn_configured, conn_seq, mask;
> -	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
> -	int i, j;
> -	bool *save_enabled;
> -	bool fallback = true, ret = true;
> +	bool fallback = true, *enabled = NULL;
> +	struct drm_client_display *display;
> +	struct drm_connector **connectors;
> +	int i, connector_count;
> +	unsigned int count;
>  	int num_connectors_enabled = 0;
>  	int num_connectors_detected = 0;
>  	struct drm_modeset_acquire_ctx ctx;
>  
> -	save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL);
> -	if (!save_enabled)
> -		return false;
> +	display = drm_client_display_create(dev);
> +	if (IS_ERR(display))
> +		return NULL;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> -	while (drm_modeset_lock_all_ctx(fb_helper->dev, &ctx) != 0)
> +	while (drm_modeset_lock_all_ctx(dev, &ctx) != 0)
>  		drm_modeset_backoff(&ctx);
>  
> -	memcpy(save_enabled, enabled, count);
> +	connector_count = drm_connector_get_all(dev, &connectors);
> +	if (connector_count < 1)
> +		goto bail;
> +
> +	enabled = drm_connector_get_enabled_status(connectors, connector_count);
> +	if (!enabled)
> +		goto bail;
> +
> +	count = min(connector_count, BITS_PER_LONG);
>  	mask = GENMASK(count - 1, 0);
>  	conn_configured = 0;
>  retry:
>  	conn_seq = conn_configured;
>  	for (i = 0; i < count; i++) {
> -		struct drm_fb_helper_connector *fb_conn;
> -		struct drm_connector *connector;
> +		struct drm_connector *connector = connectors[i];
> +		struct drm_display_mode *mode;
> +		struct drm_mode_set *modeset;
>  		struct drm_encoder *encoder;
> -		struct drm_fb_helper_crtc *new_crtc;
> -
> -		fb_conn = fb_helper->connector_info[i];
> -		connector = fb_conn->connector;
>  
>  		if (conn_configured & BIT(i))
>  			continue;
> @@ -402,16 +395,13 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  
>  		num_connectors_enabled++;
>  
> -		new_crtc = intel_fb_helper_crtc(fb_helper,
> -						connector->state->crtc);
> -
>  		/*
>  		 * Make sure we're not trying to drive multiple connectors
>  		 * with a single CRTC, since our cloning support may not
>  		 * match the BIOS.
>  		 */
> -		for (j = 0; j < count; j++) {
> -			if (crtcs[j] == new_crtc) {
> +		drm_client_display_for_each_modeset(modeset, display) {
> +			if (modeset->connectors[0] == connector) {
>  				DRM_DEBUG_KMS("fallback: cloned configuration\n");
>  				goto bail;
>  			}
> @@ -421,28 +411,26 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  			      connector->name);
>  
>  		/* go for command line mode first */
> -		modes[i] = drm_connector_pick_cmdline_mode(connector);
> +		mode = drm_connector_pick_cmdline_mode(connector);
>  
>  		/* try for preferred next */
> -		if (!modes[i]) {
> +		if (!mode) {
>  			DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
>  				      connector->name, connector->has_tile);
> -			modes[i] = drm_connector_has_preferred_mode(connector,
> -								    width,
> -								    height);
> +			mode = drm_connector_has_preferred_mode(connector,
> +								width, height);
>  		}
>  
>  		/* No preferred mode marked by the EDID? Are there any modes? */
> -		if (!modes[i] && !list_empty(&connector->modes)) {
> +		if (!mode && !list_empty(&connector->modes)) {
>  			DRM_DEBUG_KMS("using first mode listed on connector %s\n",
>  				      connector->name);
> -			modes[i] = list_first_entry(&connector->modes,
> -						    struct drm_display_mode,
> -						    head);
> +			mode = list_first_entry(&connector->modes,
> +						struct drm_display_mode, head);
>  		}
>  
>  		/* last resort: use current mode */
> -		if (!modes[i]) {
> +		if (!mode) {
>  			/*
>  			 * IMPORTANT: We want to use the adjusted mode (i.e.
>  			 * after the panel fitter upscaling) as the initial
> @@ -458,16 +446,26 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  			 */
>  			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>  				      connector->name);
> -			modes[i] = &connector->state->crtc->mode;
> +			mode = &connector->state->crtc->mode;
>  		}
> -		crtcs[i] = new_crtc;
> +
> +		modeset = drm_client_display_find_modeset(display, connector->state->crtc);
> +		if (WARN_ON(!modeset))
> +			goto bail;
> +
> +		modeset->mode = drm_mode_duplicate(dev, mode);
> +		drm_connector_get(connector);
> +		modeset->connectors[0] = connector;
> +		modeset->num_connectors = 1;
> +		modeset->x = 0;
> +		modeset->y = 0;
>  
>  		DRM_DEBUG_KMS("connector %s on [CRTC:%d:%s]: %dx%d%s\n",
>  			      connector->name,
>  			      connector->state->crtc->base.id,
>  			      connector->state->crtc->name,
> -			      modes[i]->hdisplay, modes[i]->vdisplay,
> -			      modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" :"");
> +			      mode->hdisplay, mode->vdisplay,
> +			      mode->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "");
>  
>  		fallback = false;
>  		conn_configured |= BIT(i);
> @@ -492,19 +490,20 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  	if (fallback) {
>  bail:
>  		DRM_DEBUG_KMS("Not using firmware configuration\n");
> -		memcpy(enabled, save_enabled, count);
> -		ret = false;
> +		drm_client_display_free(display);
> +		display = NULL;
>  	}
>  
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  
> -	kfree(save_enabled);
> -	return ret;
> +	drm_connector_put_all(connectors, connector_count);
> +	kfree(enabled);
> +
> +	return display;
>  }
>  
>  static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> -	.initial_config = intel_fb_initial_config,
>  	.fb_probe = intelfb_create,
>  };
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 7e545f5f94d3..13356e6fd40c 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -32,6 +32,7 @@
>  
>  #include <drm/drm_device.h>
>  
> +struct drm_client_display;
>  struct drm_file;
>  struct drm_gem_object;
>  struct drm_master;
> @@ -553,6 +554,26 @@ struct drm_driver {
>  			    struct drm_device *dev,
>  			    uint32_t handle);
>  
> +	/**
> +	 * @initial_client_config:
> +	 *
> +	 * Driver callback to setup an initial fbdev display configuration.
> +	 * Drivers can use this callback to tell the fbdev emulation what the
> +	 * preferred initial configuration is. This is useful to implement
> +	 * smooth booting where the fbdev (and subsequently all userspace) never
> +	 * changes the mode, but always inherits the existing configuration.
> +	 *
> +	 * This callback is optional.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * The driver should return true if a suitable initial configuration has
> +	 * been filled out and false when the fbdev helper should fall back to
> +	 * the default probing logic.
> +	 */
> +	struct drm_client_display *(*initial_client_display)(struct drm_device *dev,
> +					unsigned int width, unsigned int height);
> +
>  	/**
>  	 * @gem_vm_ops: Driver private ops for this object
>  	 */
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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