From patchwork Wed Jul 11 14:28:38 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1184081 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id 47157DF25A for ; Wed, 11 Jul 2012 16:18:28 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1545AA0ECD for ; Wed, 11 Jul 2012 09:18:28 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 1F1AD9E8A8 for ; Wed, 11 Jul 2012 08:36:23 -0700 (PDT) Received: by mail-wg0-f43.google.com with SMTP id dr1so946955wgb.12 for ; Wed, 11 Jul 2012 08:36:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; bh=WJdctZ3Zin+y6tn91WPj/4KS65EjXQLDvkH/xAFmxi8=; b=aRMI2ZLksiiBTq3iUvOQ6heGe7jXeFRBFeebnfUoBRYW2gQpOgUlRNgwjHWYEeeoRX I+3B4wEJRFCf2xj2N8cEE4lJ5Mk+4eIgOEvgAwh3BAT0Vkv4/kIXjtfD2f2KflY7YVSd LDy1QE5eDXXSo6+BFyE08YVoPi0nZmtSokPHY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :x-gm-message-state; bh=WJdctZ3Zin+y6tn91WPj/4KS65EjXQLDvkH/xAFmxi8=; b=RH4Mdhw7hVcaauspLMglc2i83ZRl83VA8zg3JTWf1j6NvcCE09FUXgxKPiD3i5GPan simqyIBNcCuBMAHpwhDhPdt76pXCkXwE2OjXaoUcLYDLSdmolVvmThoLzR//0CEybeN+ CTHXJhOVs5Wx2dTH0pKJjdeObJK6ocUOqsBUURZSxMN7u2iUqTohP/wOFw96ZM84h75M zyeh6/cMus9SHWFp696S7LVesDmhgbQzMf7zOGRKnVV7ekggf+wU6E27/6cltdpNwyU2 AsWoeQEUd4UmeTiVRN1ylbM6aV2tpjumlq3kEIopoeyp66EstPgm86T0mo3aKyFFODB2 jtrw== Received: by 10.180.91.1 with SMTP id ca1mr20966495wib.8.1342020983768; Wed, 11 Jul 2012 08:36:23 -0700 (PDT) Received: from wespe.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id bc2sm5777080wib.0.2012.07.11.08.36.22 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 11 Jul 2012 08:36:23 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Date: Wed, 11 Jul 2012 16:28:38 +0200 Message-Id: <1342016944-23395-56-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.7.6 In-Reply-To: <1342016944-23395-1-git-send-email-daniel.vetter@ffwll.ch> References: <1342016944-23395-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQkJx7V31C/czJ/rq6lJMptUWREgzZf0MBl0cL2+Ff2pl4A/YsvjRlTt0+Iz3wO1r8TNJUSP Cc: Daniel Vetter Subject: [Intel-gfx] [PATCH 55/81] drm/i915: stage modeset output changes X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org This is the core of the new modeset logic. The current code which is based upon the crtc helper code first updates all the link of the new display pipeline and then calls the lower-level set_mode function to execute the required callbacks to get there. The issue with this approach is that for disabling we need to know the _current_ display pipe state, not the new one. Hence we need to stage the new state of the display pipe and only update it once we have disabled the current configuration and before we start to update the hw registers with the new configuration. This patch here just prepares the ground by switching the new output state computation to these staging pointers. To make it clearer, rename the old update_output_state function to stage_output_state. A few peculiarities: - We're also calling the set_mode function at various places to update properties. Hence after a successfule modeset we need to stage the current configuration (for otherwise we might fall back again). This happens automatically because as part of the (successful) modeset we need to copy the staged state to the real one. But for the hw readout code we need to make sure that this happens, too. - Teach the new staged output state computation code the required smarts to handle the disabling of outputs. The current code handles this in a special case, but to better handle global modeset changes covering more than one crtc, we want to do this all in the same low-level modeset code. - The actual modeset code is still a bit ugly and wants to know the new crtc->enabled state a bit early. Follow-on patches will clean that up, for now we have to apply the staged output configuration early, outside of the set_mode functions. - Improve/add comments in stage_output_state. Essentially all that is left to do now is move the disabling code into set_mode and then move the staged state update code also into set_mode, at the right place between disabling things and calling the mode_set callbacks for the new configuration. v2: Disabling a crtc works by passing in a NULL mode or fb, userspace doesn't hand in the list of connectors. We therefore need to detect this case manually and tear down all the output links. v3: Properly update the output staging pointers after having read out the hw state. v4: Simplify the code, add more DRM_DEBUG_KMS output and check a few assumptions with WARN_ON. Essentially all things that I've noticed while debugging issues in other places of the code. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_drv.h | 16 +++ 2 files changed, 142 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e2563d0..c5bf814 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6619,6 +6619,51 @@ intel_crtc_prepare_encoders(struct drm_device *dev) } } +/** + * intel_modeset_update_staged_output_state + * + * Updates the staged output configuration state, e.g. after we've read out the + * current hw state. + */ +static void intel_modeset_update_staged_output_state(struct drm_device *dev) +{ + struct intel_encoder *encoder; + struct intel_connector *connector; + + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + connector->new_encoder = + to_intel_encoder(connector->base.encoder); + } + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, + base.head) { + encoder->new_crtc = + to_intel_crtc(encoder->base.crtc); + } +} + +/** + * intel_modeset_commit_output_state + * + * This function copies the stage display pipe configuration to the real one. + */ +static void intel_modeset_commit_output_state(struct drm_device *dev) +{ + struct intel_encoder *encoder; + struct intel_connector *connector; + + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + connector->base.encoder = &connector->new_encoder->base; + } + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, + base.head) { + encoder->base.crtc = &encoder->new_crtc->base; + } +} + bool intel_crtc_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, @@ -6785,8 +6830,8 @@ static void intel_set_config_restore_state(struct drm_device *dev, struct intel_set_config *config) { struct drm_crtc *crtc; - struct drm_encoder *encoder; - struct drm_connector *connector; + struct intel_encoder *encoder; + struct intel_connector *connector; int count; count = 0; @@ -6795,13 +6840,15 @@ static void intel_set_config_restore_state(struct drm_device *dev, } count = 0; - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - encoder->crtc = config->save_encoder_crtcs[count++]; + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { + encoder->new_crtc = + to_intel_crtc(config->save_encoder_crtcs[count++]); } count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - connector->encoder = config->save_connector_encoders[count++]; + list_for_each_entry(connector, &dev->mode_config.connector_list, base.head) { + connector->new_encoder = + to_intel_encoder(config->save_connector_encoders[count++]); } } @@ -6840,73 +6887,102 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set, } static int -intel_set_config_update_output_state(struct drm_device *dev, - struct drm_mode_set *set, - struct intel_set_config *config) +intel_modeset_stage_output_state(struct drm_device *dev, + struct drm_mode_set *set, + struct intel_set_config *config) { struct drm_crtc *new_crtc; - struct drm_encoder *new_encoder; - struct drm_connector *connector; + struct intel_connector *connector; + struct intel_encoder *encoder; int count, ro; - /* a) traverse passed in connector list and get encoders for them */ + /* The upper layers ensure that we either disabl a crtc or have a list + * of connectors. For paranoia, double-check this. */ + WARN_ON(!set->fb && (set->num_connectors != 0)); + WARN_ON(set->fb && (set->num_connectors == 0)); + count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - new_encoder = connector->encoder; + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + /* If we disable the crtc, disable all its connectors. */ + if (!set->fb && connector->base.encoder && + connector->base.encoder->crtc == set->crtc) { + connector->new_encoder = NULL; + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [NOCRTC]\n", + connector->base.base.id, + drm_get_connector_name(&connector->base)); + } + + /* Otherwise traverse passed in connector list and get encoders + * for them. */ for (ro = 0; ro < set->num_connectors; ro++) { - if (set->connectors[ro] == connector) { - new_encoder = - &intel_attached_encoder(connector)->base; + if (set->connectors[ro] == &connector->base) { + connector->new_encoder = connector->encoder; break; } } - if (new_encoder != connector->encoder) { + if (&connector->new_encoder->base != connector->base.encoder) { DRM_DEBUG_KMS("encoder changed, full mode switch\n"); config->mode_changed = true; - /* If the encoder is reused for another connector, then - * the appropriate crtc will be set later. - */ - if (connector->encoder) - connector->encoder->crtc = NULL; - connector->encoder = new_encoder; } + + /* Disable all disconnected encoders. */ + if (connector->base.status == connector_status_disconnected) + connector->new_encoder = NULL; } + /* connector->new_encoder is now updated for all connectors. */ + /* Update crtc of enabled connectors. */ count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - if (!connector->encoder) + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + if (!connector->new_encoder) continue; - if (connector->encoder->crtc == set->crtc) - new_crtc = NULL; - else - new_crtc = connector->encoder->crtc; + new_crtc = connector->new_encoder->base.crtc; for (ro = 0; ro < set->num_connectors; ro++) { - if (set->connectors[ro] == connector) + if (set->connectors[ro] == &connector->base) new_crtc = set->crtc; } /* Make sure the new CRTC will work with the encoder */ - if (new_crtc && - !intel_encoder_crtc_ok(connector->encoder, new_crtc)) { + if (!intel_encoder_crtc_ok(&connector->new_encoder->base, + new_crtc)) { return -EINVAL; } - if (new_crtc != connector->encoder->crtc) { + connector->encoder->new_crtc = to_intel_crtc(new_crtc); + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n", + connector->base.base.id, + drm_get_connector_name(&connector->base), + new_crtc->base.id); + } + + /* Check for any encoders that needs to be disabled. */ + list_for_each_entry(encoder, &dev->mode_config.encoder_list, + base.head) { + list_for_each_entry(connector, + &dev->mode_config.connector_list, + base.head) { + if (connector->new_encoder == encoder) { + WARN_ON(!connector->new_encoder->new_crtc); + + goto next_encoder; + } + } + encoder->new_crtc = NULL; +next_encoder: + /* Only now check for crtc changes so we don't miss encoders + * that will be disabled. */ + if (&encoder->new_crtc->base != encoder->base.crtc) { DRM_DEBUG_KMS("crtc changed, full mode switch\n"); config->mode_changed = true; - connector->encoder->crtc = new_crtc; - } - if (new_crtc) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n", - connector->base.id, drm_get_connector_name(connector), - new_crtc->base.id); - } else { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [NOCRTC]\n", - connector->base.id, drm_get_connector_name(connector)); } } + /* Now we've also updated encoder->new_crtc for all encoders. */ return 0; } @@ -6965,11 +7041,13 @@ static int intel_crtc_set_config(struct drm_mode_set *set) * such cases. */ intel_set_config_compute_mode_changes(set, config); - ret = intel_set_config_update_output_state(dev, set, config); + ret = intel_modeset_stage_output_state(dev, set, config); if (ret) goto fail; if (config->mode_changed) { + intel_modeset_commit_output_state(dev); + set->crtc->enabled = drm_helper_crtc_in_use(set->crtc); if (set->crtc->enabled) { DRM_DEBUG_KMS("attempting to set mode from" @@ -7016,6 +7094,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set) fail: intel_set_config_restore_state(dev, config); + intel_modeset_commit_output_state(dev); + /* Try to restore the config */ if (config->mode_changed && !intel_crtc_set_mode(save_set.crtc, save_set.mode, save_set.x, @@ -7915,6 +7995,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev) crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); intel_sanitize_crtc(crtc); } + + intel_modeset_update_staged_output_state(dev); } void intel_modeset_gem_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1065c2d..ff8760b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -131,6 +131,12 @@ struct intel_fbdev { struct intel_encoder { struct drm_encoder base; + /* + * The new crtc this encoder will be driven from. Only differs from + * base->crtc while a modeset is in progress. + */ + struct intel_crtc *new_crtc; + int type; bool needs_tv_clock; bool connectors_active; @@ -151,7 +157,17 @@ struct intel_encoder { struct intel_connector { struct drm_connector base; + /* + * The fixed encoder this connector is connected to. + */ struct intel_encoder *encoder; + + /* + * The new encoder this connector will be driven. Only differs from + * encoder while a modeset is in progress. + */ + struct intel_encoder *new_encoder; + /* Reads out the current hw, returning true if the connector is enabled * and active (i.e. dpms ON state). */ bool (*get_hw_state)(struct intel_connector *);