On Fri, Jan 31, 2020 at 09:15:46AM -0800, Manasi Navare wrote:
> This patch pushes out the computation of master and slave
> transcoders in crtc states after encoder's compute_config hook.
> This ensures that the assigned master slave crtcs have exact same
> mode and timings which is a requirement for Port sync mode
> to be enabled.
>
> Suggested-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 125 +++++++++++++++
> drivers/gpu/drm/i915/display/intel_display.c | 156 -------------------
> 2 files changed, 125 insertions(+), 156 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index c96f629cddc3..4ec36eb5ce28 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4440,6 +4440,130 @@ static int intel_ddi_compute_config(struct
> intel_encoder *encoder,
> return 0;
> }
>
> +static bool mode_equal(const struct drm_display_mode *mode1,
> + const struct drm_display_mode *mode2)
> +{
> + return drm_mode_match(mode1, mode2,
> + DRM_MODE_MATCH_TIMINGS |
> + DRM_MODE_MATCH_FLAGS |
> + DRM_MODE_MATCH_3D_FLAGS) &&
> + mode1->clock == mode2->clock; /* we want an exact match */
Trying to remember why we chose .clock rather than .crtc_clock... Ah,
because drm_mode_match() anyway looks at the non-crtc timings. Yeah,
should work fine since we make sure the 3D flags match so the crtc
timings vs non-crtc timings should be consistent for each one.
> +}
> +
> +static bool m_n_equal(const struct intel_link_m_n *m_n_1,
> + const struct intel_link_m_n *m_n_2)
> +{
> + return m_n_1->tu == m_n_2->tu &&
> + m_n_1->gmch_m == m_n_2->gmch_m &&
> + m_n_1->gmch_n == m_n_2->gmch_n &&
> + m_n_1->link_m == m_n_2->link_m &&
> + m_n_1->link_n == m_n_2->link_n;
> +}
> +
> +static bool crtcs_port_sync_compatible(struct intel_crtc_state *crtc_state1,
> + struct intel_crtc_state *crtc_state2)
> +{
> + return crtc_state1->hw.active && crtc_state2->hw.active &&
> + crtc_state1->output_types == crtc_state2->output_types &&
> + crtc_state1->output_format == crtc_state2->output_format &&
> + crtc_state1->lane_count == crtc_state2->lane_count &&
> + crtc_state1->port_clock == crtc_state2->port_clock &&
> + mode_equal(&crtc_state1->hw.adjusted_mode,
> + &crtc_state2->hw.adjusted_mode) &&
> + m_n_equal(&crtc_state1->dp_m_n, &crtc_state2->dp_m_n);
> +}
> +
> +static u8
> +intel_ddi_compute_port_sync_crtc_state
The name is misleading. something_port_sync_transcoders() etc. to
better describe what it returns.
> (struct drm_connector *ref_connector,
You don't need the whole connector. The tile group id will suffice.
> + struct intel_crtc_state *ref_crtc_state)
const
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_i915_private *dev_priv =
> to_i915(ref_crtc_state->uapi.crtc->dev);
> + struct intel_atomic_state *state =
> + to_intel_atomic_state(ref_crtc_state->uapi.state);
> + struct intel_crtc *ref_crtc = to_intel_crtc(ref_crtc_state->uapi.crtc);
> + u8 transcoders = 0;
> + int i;
> +
> + DRM_DEBUG_KMS("for [CRTC:%d:%s]\n",
> + ref_crtc_state->uapi.crtc->base.id,
> + ref_crtc_state->uapi.crtc->name);
More noise than signal. Pls remove.
> +
> + if (INTEL_GEN(dev_priv) < 11)
> + return 0;
> +
> + if (!intel_crtc_has_type(ref_crtc_state, INTEL_OUTPUT_DP))
> + return 0;
> +
> + for_each_new_connector_in_state(&state->base, connector, conn_state, i)
> {
> + struct intel_crtc *crtc = to_intel_crtc(conn_state->crtc);
> + struct intel_crtc_state *crtc_state = NULL;
> +
> + if (!crtc)
> + continue;
> +
> + if (!(connector->has_tile &&
> + connector->tile_group->id ==
> + ref_connector->tile_group->id))
> + continue;
> +
> + crtc_state = intel_atomic_get_new_crtc_state(state,
> + crtc);
> + if (!crtcs_port_sync_compatible(ref_crtc_state,
> + crtc_state)) {
> + DRM_DEBUG_KMS("[CRTC:%d:%s] and [CRTC:%d:%s] not Port
> Sync Compatible\n",
> + ref_crtc->base.base.id,
> + ref_crtc->base.name,
> + crtc->base.base.id, crtc->base.name);
Another noisy debug. The state dump will tell us everything we need to
know. If not we should improve the state dump instead.
> + continue;
> + }
> +
> + transcoders |= BIT(crtc_state->cpu_transcoder);
> + }
> +
> + return transcoders;
indent fail
> +}
> +
> +static int intel_ddi_compute_config_late(struct intel_encoder *encoder,
> + struct intel_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_connector *connector = conn_state->connector;
> + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> + u8 port_sync_transcoders = 0;
> +
> + DRM_DEBUG_KMS("[ENCODER:%d:%s] [CRTC:%d:%s]",
> + encoder->base.base.id, encoder->base.name,
> + crtc_state->uapi.crtc->base.id,
> crtc_state->uapi.crtc->name);
> +
> + if (connector->has_tile)
> + port_sync_transcoders =
> intel_ddi_compute_port_sync_crtc_state(connector,
> +
> crtc_state);
> +
> + /*
> + * EDP Transcoders cannot be ensalved
> + * make them a master always when present
> + */
> + if (port_sync_transcoders & BIT(TRANSCODER_EDP))
> + crtc_state->master_transcoder = TRANSCODER_EDP;
> + else
> + crtc_state->master_transcoder = ffs(port_sync_transcoders) - 1;
> +
> + if (crtc_state->master_transcoder == crtc_state->cpu_transcoder) {
> + crtc_state->master_transcoder = INVALID_TRANSCODER;
> + crtc_state->sync_mode_slaves_mask =
> + port_sync_transcoders &
> ~BIT(crtc_state->cpu_transcoder);
> + }
> +
> + drm_dbg_kms(&dev_priv->drm,
> + "Master Transcoder = %s , Slave transcoder bitmask =
> 0x%08x\n",
> + transcoder_name(crtc_state->master_transcoder),
> + crtc_state->sync_mode_slaves_mask);
Redundant. Again it's part of the state dump anyway. Pls remove.
> +
> + return 0;
> +}
> +
> static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
> {
> struct intel_digital_port *dig_port =
> enc_to_dig_port(to_intel_encoder(encoder));
> @@ -4749,6 +4873,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv,
> enum port port)
> encoder->hotplug = intel_ddi_hotplug;
> encoder->compute_output_type = intel_ddi_compute_output_type;
> encoder->compute_config = intel_ddi_compute_config;
> + encoder->compute_config_late = intel_ddi_compute_config_late;
> encoder->enable = intel_enable_ddi;
> encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
> encoder->pre_enable = intel_ddi_pre_enable;
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index de4ab51216f6..e638543f5f87 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12545,126 +12545,6 @@ static bool c8_planes_changed(const struct
> intel_crtc_state *new_crtc_state)
> return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> }
>
> -static bool
> -intel_atomic_is_master_connector(struct intel_crtc_state *crtc_state)
> -{
> - struct drm_crtc *crtc = crtc_state->uapi.crtc;
> - struct drm_atomic_state *state = crtc_state->uapi.state;
> - struct drm_connector *connector;
> - struct drm_connector_state *connector_state;
> - int i;
> -
> - for_each_new_connector_in_state(state, connector, connector_state, i) {
> - if (connector_state->crtc != crtc)
> - continue;
> - if (connector->has_tile &&
> - connector->tile_h_loc == connector->num_h_tile - 1 &&
> - connector->tile_v_loc == connector->num_v_tile - 1)
> - return true;
> - }
> -
> - return false;
> -}
> -
> -static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> -{
> - crtc_state->master_transcoder = INVALID_TRANSCODER;
> - crtc_state->sync_mode_slaves_mask = 0;
> -}
> -
> -static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> - struct intel_crtc_state *crtc_state,
> - int num_tiled_conns)
> -{
> - struct drm_crtc *crtc = crtc_state->uapi.crtc;
> - struct intel_atomic_state *state =
> to_intel_atomic_state(crtc_state->uapi.state);
> - struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> - struct drm_connector *master_connector;
> - struct drm_connector_list_iter conn_iter;
> - struct drm_crtc *master_crtc = NULL;
> - struct drm_crtc_state *master_crtc_state;
> - struct intel_crtc_state *master_pipe_config;
> -
> - if (INTEL_GEN(dev_priv) < 11)
> - return 0;
> -
> - if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> - return 0;
> -
> - /*
> - * In case of tiled displays there could be one or more slaves but
> there is
> - * only one master. Lets make the CRTC used by the connector
> corresponding
> - * to the last horizonal and last vertical tile a master/genlock CRTC.
> - * All the other CRTCs corresponding to other tiles of the same Tile
> group
> - * are the slave CRTCs and hold a pointer to their genlock CRTC.
> - * If all tiles not present do not make master slave assignments.
> - */
> - if (!connector->has_tile ||
> - crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> - crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> - num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> - reset_port_sync_mode_state(crtc_state);
> - return 0;
> - }
> - /* Last Horizontal and last vertical tile connector is a master
> - * Master's crtc state is already populated in slave for port sync
> - */
> - if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> - connector->tile_v_loc == connector->num_v_tile - 1)
> - return 0;
> -
> - /* Loop through all connectors and configure the Slave crtc_state
> - * to point to the correct master.
> - */
> - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> - drm_for_each_connector_iter(master_connector, &conn_iter) {
> - struct drm_connector_state *master_conn_state = NULL;
> -
> - if (!(master_connector->has_tile &&
> - master_connector->tile_group->id ==
> connector->tile_group->id))
> - continue;
> - if (master_connector->tile_h_loc !=
> master_connector->num_h_tile - 1 ||
> - master_connector->tile_v_loc !=
> master_connector->num_v_tile - 1)
> - continue;
> -
> - master_conn_state = drm_atomic_get_connector_state(&state->base,
> -
> master_connector);
> - if (IS_ERR(master_conn_state)) {
> - drm_connector_list_iter_end(&conn_iter);
> - return PTR_ERR(master_conn_state);
> - }
> - if (master_conn_state->crtc) {
> - master_crtc = master_conn_state->crtc;
> - break;
> - }
> - }
> - drm_connector_list_iter_end(&conn_iter);
> -
> - if (!master_crtc) {
> - drm_dbg_kms(&dev_priv->drm,
> - "Could not find Master CRTC for Slave CRTC %d\n",
> - crtc->base.id);
> - return -EINVAL;
> - }
> -
> - master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> - master_crtc);
> - if (IS_ERR(master_crtc_state))
> - return PTR_ERR(master_crtc_state);
> -
> - master_pipe_config = to_intel_crtc_state(master_crtc_state);
> - crtc_state->master_transcoder = master_pipe_config->cpu_transcoder;
> - master_pipe_config->sync_mode_slaves_mask |=
> - BIT(crtc_state->cpu_transcoder);
> - drm_dbg_kms(&dev_priv->drm,
> - "Master Transcoder = %s added for Slave CRTC = %d, slave
> transcoder bitmask = %d\n",
> - transcoder_name(crtc_state->master_transcoder),
> - crtc->base.id,
> - master_pipe_config->sync_mode_slaves_mask);
> -
> - return 0;
> -}
> -
> static int intel_crtc_atomic_check(struct intel_atomic_state *state,
> struct intel_crtc *crtc)
> {
> @@ -13226,15 +13106,6 @@ intel_crtc_prepare_cleared_state(struct
> intel_crtc_state *crtc_state)
> if (IS_G4X(dev_priv) ||
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> saved_state->wm = crtc_state->wm;
> - /*
> - * Save the slave bitmask which gets filled for master crtc state during
> - * slave atomic check call. For all other CRTCs reset the port sync
> variables
> - * crtc_state->master_transcoder needs to be set to INVALID
> - */
> - reset_port_sync_mode_state(saved_state);
> - if (intel_atomic_is_master_connector(crtc_state))
> - saved_state->sync_mode_slaves_mask =
> - crtc_state->sync_mode_slaves_mask;
>
> memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> kfree(saved_state);
> @@ -13325,24 +13196,6 @@ intel_modeset_pipe_config(struct intel_crtc_state
> *pipe_config)
> drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
> CRTC_STEREO_DOUBLE);
>
> - /* Get tile_group_id of tiled connector */
> - for_each_new_connector_in_state(state, connector, connector_state, i) {
> - if (connector_state->crtc == crtc &&
> - connector->has_tile) {
> - tile_group_id = connector->tile_group->id;
> - break;
> - }
> - }
> -
> - /* Get total number of tiled connectors in state that belong to
> - * this tile group.
> - */
> - for_each_new_connector_in_state(state, connector, connector_state, i) {
> - if (connector->has_tile &&
> - connector->tile_group->id == tile_group_id)
> - num_tiled_conns++;
> - }
> -
> /* Pass our mode to the connectors and the CRTC to give them a chance to
> * adjust it according to limitations or connector properties, and also
> * a chance to reject the mode entirely.
> @@ -13354,15 +13207,6 @@ intel_modeset_pipe_config(struct intel_crtc_state
> *pipe_config)
> if (connector_state->crtc != crtc)
> continue;
>
> - ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> - num_tiled_conns);
> - if (ret) {
> - drm_dbg_kms(&i915->drm,
> - "Cannot assign Sync Mode CRTCs: %d\n",
> - ret);
> - return ret;
> - }
> -
> ret = encoder->compute_config(encoder, pipe_config,
> connector_state);
> if (ret < 0) {
> --
> 2.19.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
