[Intel-gfx] [PATCH 06/16] drm/i915/fbdev: Move intel_fb_initial_config() to fbdev helper

Noralf Trønnes noralf at tronnes.org
Sat Mar 30 20:50:05 UTC 2019



Den 27.03.2019 14.33, skrev Jani Nikula:
> On Tue, 26 Mar 2019, Noralf Trønnes <noralf at tronnes.org> wrote:
>> It is generic code and having it in the helper will let other drivers
>> benefit from it.
>>
>> One change was necessary assuming this to be true:
>> INTEL_INFO(dev_priv)->num_pipes == dev->mode_config.num_crtc
> 
> This holds after intel_modeset_init(), in time for the use here.
> 
>> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: intel-gfx at lists.freedesktop.org
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> 
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
> 
> and
> 
> Acked-by: Jani Nikula <jani.nikula at intel.com>
> 
> for merging via drm-misc or whichever tree makes most sense.
> 

Thanks, I'll take it through drm-misc.

One of the patches had a conflict with drm-tip, so I didn't get a CI
run. I'll apply this if CI agrees when I submit the next version.

Noralf.

> 
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c    | 194 ++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_fbdev.c | 218 -----------------------------
>>  include/drm/drm_fb_helper.h        |  23 ---
>>  3 files changed, 190 insertions(+), 245 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 36310901e935..634f4dcf0c41 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2545,6 +2545,194 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>>  	return best_score;
>>  }
>>  
>> +static struct drm_fb_helper_crtc *
>> +drm_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 config */
>> +static bool drm_fb_helper_firmware_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_device *dev = fb_helper->dev;
>> +	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
>> +	unsigned long conn_configured, conn_seq;
>> +	int i, j;
>> +	bool *save_enabled;
>> +	bool fallback = true, ret = true;
>> +	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;
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	while (drm_modeset_lock_all_ctx(dev, &ctx) != 0)
>> +		drm_modeset_backoff(&ctx);
>> +
>> +	memcpy(save_enabled, enabled, count);
>> +	conn_seq = GENMASK(count - 1, 0);
>> +	conn_configured = 0;
>> +retry:
>> +	for (i = 0; i < count; i++) {
>> +		struct drm_fb_helper_connector *fb_conn;
>> +		struct drm_connector *connector;
>> +		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;
>> +
>> +		/* First pass, only consider tiled connectors */
>> +		if (conn_seq == GENMASK(count - 1, 0) && !connector->has_tile)
>> +			continue;
>> +
>> +		if (connector->status == connector_status_connected)
>> +			num_connectors_detected++;
>> +
>> +		if (!enabled[i]) {
>> +			DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
>> +				      connector->name);
>> +			conn_configured |= BIT(i);
>> +			continue;
>> +		}
>> +
>> +		if (connector->force == DRM_FORCE_OFF) {
>> +			DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
>> +				      connector->name);
>> +			enabled[i] = false;
>> +			continue;
>> +		}
>> +
>> +		encoder = connector->state->best_encoder;
>> +		if (!encoder || WARN_ON(!connector->state->crtc)) {
>> +			if (connector->force > DRM_FORCE_OFF)
>> +				goto bail;
>> +
>> +			DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
>> +				      connector->name);
>> +			enabled[i] = false;
>> +			conn_configured |= BIT(i);
>> +			continue;
>> +		}
>> +
>> +		num_connectors_enabled++;
>> +
>> +		new_crtc = drm_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_DEBUG_KMS("fallback: cloned configuration\n");
>> +				goto bail;
>> +			}
>> +		}
>> +
>> +		DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
>> +			      connector->name);
>> +
>> +		/* go for command line mode first */
>> +		modes[i] = drm_pick_cmdline_mode(fb_conn);
>> +
>> +		/* try for preferred next */
>> +		if (!modes[i]) {
>> +			DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
>> +				      connector->name, connector->has_tile);
>> +			modes[i] = drm_has_preferred_mode(fb_conn, width,
>> +							  height);
>> +		}
>> +
>> +		/* No preferred mode marked by the EDID? Are there any modes? */
>> +		if (!modes[i] && !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);
>> +		}
>> +
>> +		/* last resort: use current mode */
>> +		if (!modes[i]) {
>> +			/*
>> +			 * IMPORTANT: We want to use the adjusted mode (i.e.
>> +			 * after the panel fitter upscaling) as the initial
>> +			 * config, not the input mode, which is what crtc->mode
>> +			 * usually contains. But since our current
>> +			 * code puts a mode derived from the post-pfit timings
>> +			 * into crtc->mode this works out correctly.
>> +			 *
>> +			 * This is crtc->mode and not crtc->state->mode for the
>> +			 * fastboot check to work correctly.
>> +			 */
>> +			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>> +				      connector->name);
>> +			modes[i] = &connector->state->crtc->mode;
>> +		}
>> +		crtcs[i] = new_crtc;
>> +
>> +		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" : "");
>> +
>> +		fallback = false;
>> +		conn_configured |= BIT(i);
>> +	}
>> +
>> +	if (conn_configured != conn_seq) { /* repeat until no more are found */
>> +		conn_seq = conn_configured;
>> +		goto retry;
>> +	}
>> +
>> +	/*
>> +	 * If the BIOS didn't enable everything it could, fall back to have the
>> +	 * same user experiencing of lighting up as much as possible like the
>> +	 * fbdev helper library.
>> +	 */
>> +	if (num_connectors_enabled != num_connectors_detected &&
>> +	    num_connectors_enabled < dev->mode_config.num_crtc) {
>> +		DRM_DEBUG_KMS("fallback: Not all outputs enabled\n");
>> +		DRM_DEBUG_KMS("Enabled: %i, detected: %i\n", num_connectors_enabled,
>> +			      num_connectors_detected);
>> +		fallback = true;
>> +	}
>> +
>> +	if (fallback) {
>> +bail:
>> +		DRM_DEBUG_KMS("Not using firmware configuration\n");
>> +		memcpy(enabled, save_enabled, count);
>> +		ret = false;
>> +	}
>> +
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +
>> +	kfree(save_enabled);
>> +	return ret;
>> +}
>> +
>>  static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>  			    u32 width, u32 height)
>>  {
>> @@ -2577,10 +2765,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>  		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>>  	drm_enable_connectors(fb_helper, enabled);
>>  
>> -	if (!(fb_helper->funcs->initial_config &&
>> -	      fb_helper->funcs->initial_config(fb_helper, crtcs, modes,
>> -					       offsets,
>> -					       enabled, width, height))) {
>> +	if (!drm_fb_helper_firmware_config(fb_helper, crtcs, modes, offsets,
>> +					   enabled, width, height)) {
>>  		memset(modes, 0, fb_helper->connector_count*sizeof(modes[0]));
>>  		memset(crtcs, 0, fb_helper->connector_count*sizeof(crtcs[0]));
>>  		memset(offsets, 0, fb_helper->connector_count*sizeof(offsets[0]));
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index e8f694b57b8a..20e91600cf71 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -292,225 +292,7 @@ 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.
>> - *
>> - * The BIOS or boot loader will generally create an initial display
>> - * configuration for us that includes some set of active pipes and displays.
>> - * This routine tries to figure out which pipes and connectors are active
>> - * and stuffs them into the crtcs and modes array given to us by the
>> - * drm_fb_helper code.
>> - *
>> - * The overall sequence is:
>> - *   intel_fbdev_init - from driver load
>> - *     intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
>> - *     drm_fb_helper_init - build fb helper structs
>> - *     drm_fb_helper_single_add_all_connectors - more fb helper structs
>> - *   intel_fbdev_initial_config - apply the config
>> - *     drm_fb_helper_initial_config - call ->probe then register_framebuffer()
>> - *         drm_setup_crtcs - build crtc config for fbdev
>> - *           intel_fb_initial_config - find active connectors etc
>> - *         drm_fb_helper_single_fb_probe - set up fbdev
>> - *           intelfb_create - re-use or alloc fb, build out fbdev structs
>> - *
>> - * Note that we don't make special consideration whether we could actually
>> - * switch to the selected modes without a full modeset. E.g. when the display
>> - * 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_i915_private *dev_priv = to_i915(fb_helper->dev);
>> -	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
>> -	unsigned long conn_configured, conn_seq;
>> -	int i, j;
>> -	bool *save_enabled;
>> -	bool fallback = true, ret = true;
>> -	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;
>> -
>> -	drm_modeset_acquire_init(&ctx, 0);
>> -
>> -	while (drm_modeset_lock_all_ctx(fb_helper->dev, &ctx) != 0)
>> -		drm_modeset_backoff(&ctx);
>> -
>> -	memcpy(save_enabled, enabled, count);
>> -	conn_seq = GENMASK(count - 1, 0);
>> -	conn_configured = 0;
>> -retry:
>> -	for (i = 0; i < count; i++) {
>> -		struct drm_fb_helper_connector *fb_conn;
>> -		struct drm_connector *connector;
>> -		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;
>> -
>> -		/* First pass, only consider tiled connectors */
>> -		if (conn_seq == GENMASK(count - 1, 0) && !connector->has_tile)
>> -			continue;
>> -
>> -		if (connector->status == connector_status_connected)
>> -			num_connectors_detected++;
>> -
>> -		if (!enabled[i]) {
>> -			DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
>> -				      connector->name);
>> -			conn_configured |= BIT(i);
>> -			continue;
>> -		}
>> -
>> -		if (connector->force == DRM_FORCE_OFF) {
>> -			DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
>> -				      connector->name);
>> -			enabled[i] = false;
>> -			continue;
>> -		}
>> -
>> -		encoder = connector->state->best_encoder;
>> -		if (!encoder || WARN_ON(!connector->state->crtc)) {
>> -			if (connector->force > DRM_FORCE_OFF)
>> -				goto bail;
>> -
>> -			DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
>> -				      connector->name);
>> -			enabled[i] = false;
>> -			conn_configured |= BIT(i);
>> -			continue;
>> -		}
>> -
>> -		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_DEBUG_KMS("fallback: cloned configuration\n");
>> -				goto bail;
>> -			}
>> -		}
>> -
>> -		DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
>> -			      connector->name);
>> -
>> -		/* go for command line mode first */
>> -		modes[i] = drm_pick_cmdline_mode(fb_conn);
>> -
>> -		/* try for preferred next */
>> -		if (!modes[i]) {
>> -			DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
>> -				      connector->name, connector->has_tile);
>> -			modes[i] = drm_has_preferred_mode(fb_conn, width,
>> -							  height);
>> -		}
>> -
>> -		/* No preferred mode marked by the EDID? Are there any modes? */
>> -		if (!modes[i] && !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);
>> -		}
>> -
>> -		/* last resort: use current mode */
>> -		if (!modes[i]) {
>> -			/*
>> -			 * IMPORTANT: We want to use the adjusted mode (i.e.
>> -			 * after the panel fitter upscaling) as the initial
>> -			 * config, not the input mode, which is what crtc->mode
>> -			 * usually contains. But since our current
>> -			 * code puts a mode derived from the post-pfit timings
>> -			 * into crtc->mode this works out correctly.
>> -			 *
>> -			 * This is crtc->mode and not crtc->state->mode for the
>> -			 * fastboot check to work correctly. crtc_state->mode has
>> -			 * I915_MODE_FLAG_INHERITED, which we clear to force check
>> -			 * state.
>> -			 */
>> -			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>> -				      connector->name);
>> -			modes[i] = &connector->state->crtc->mode;
>> -		}
>> -		crtcs[i] = new_crtc;
>> -
>> -		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" :"");
>> -
>> -		fallback = false;
>> -		conn_configured |= BIT(i);
>> -	}
>> -
>> -	if (conn_configured != conn_seq) { /* repeat until no more are found */
>> -		conn_seq = conn_configured;
>> -		goto retry;
>> -	}
>> -
>> -	/*
>> -	 * If the BIOS didn't enable everything it could, fall back to have the
>> -	 * same user experiencing of lighting up as much as possible like the
>> -	 * fbdev helper library.
>> -	 */
>> -	if (num_connectors_enabled != num_connectors_detected &&
>> -	    num_connectors_enabled < INTEL_INFO(dev_priv)->num_pipes) {
>> -		DRM_DEBUG_KMS("fallback: Not all outputs enabled\n");
>> -		DRM_DEBUG_KMS("Enabled: %i, detected: %i\n", num_connectors_enabled,
>> -			      num_connectors_detected);
>> -		fallback = true;
>> -	}
>> -
>> -	if (fallback) {
>> -bail:
>> -		DRM_DEBUG_KMS("Not using firmware configuration\n");
>> -		memcpy(enabled, save_enabled, count);
>> -		ret = false;
>> -	}
>> -
>> -	drm_modeset_drop_locks(&ctx);
>> -	drm_modeset_acquire_fini(&ctx);
>> -
>> -	kfree(save_enabled);
>> -	return ret;
>> -}
>> -
>>  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_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 7a095964f6b2..bca4b34dc93b 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -100,29 +100,6 @@ struct drm_fb_helper_funcs {
>>  	 */
>>  	int (*fb_probe)(struct drm_fb_helper *helper,
>>  			struct drm_fb_helper_surface_size *sizes);
>> -
>> -	/**
>> -	 * @initial_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.
>> -	 */
>> -	bool (*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_fb_helper_connector {
> 


More information about the Intel-gfx mailing list