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

Den 27.03.2019 14.33, skrev Jani Nikula:

On Tue, 26 Mar 2019, Noralf Trønnes noralf@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@ffwll.ch Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Noralf Trønnes noralf@tronnes.org

Reviewed-by: Jani Nikula jani.nikula@intel.com

and

Acked-by: Jani Nikula jani.nikula@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 {