[Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Dec 20 13:59:41 UTC 2019
On Thu, Dec 19, 2019 at 01:51:16PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
>
> v2:
> * Rename icl_add_sync_mode_crtcs
> * Move this function just before .compute_config hook
> * Check if DP before master slave assignments (Ville)
>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
> 1 file changed, 99 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 00608d8cef50..9c1b1256be68 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12014,88 +12014,106 @@ 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 int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> +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, *connector;
> - struct drm_connector_state *connector_state;
> + 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;
> - int i, tile_group_id;
>
> 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.
> + *
> + * FIXME: Add support for multiple tile grp ids in the future when such
> + * panels are available.
What does that mean? Why would we treat mutliple groups as a single
tiled display?
> */
> - for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> - if (connector_state->crtc != crtc)
> - continue;
> - if (!connector->has_tile)
> + 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);
I really don't understand this back and forth of setting/clearing
the master/slave state. I think the way to make it not convoluted is
to just clear everything in intel_crtc_prepare_cleared_state() and
then just compute everything from scratch.
> + return 0;
> + }
> + /* Last Horizontal and last vertical tile connector is a master
> + * Master Trans for a Master CRTC is always INVALID.
> + */
> + if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> + connector->tile_v_loc == connector->num_v_tile - 1) {
> + crtc_state->master_transcoder = INVALID_TRANSCODER;
> + return 0;
> + }
> +
> + /* Loop through all connectors and configure the Slave crtc_state
> + * to point to the correct master.
> + */
> + reset_port_sync_mode_state(crtc_state);
> + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> + drm_for_each_connector_iter(master_connector, &conn_iter) {
Why are we all of a sudden iterating through all connectors? I think this
should just iterate the connectors already in the state. The only place
where we want to look at the full connector list is the modeset_all_tiles()
thing.
> + struct drm_connector_state *master_conn_state = NULL;
> +
> + if (!(master_connector->has_tile &&
> + master_connector->tile_group->id == connector->tile_group->id))
> continue;
> - if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> - return 0;
> - if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> - connector->tile_v_loc == connector->num_v_tile - 1)
> + if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> + master_connector->tile_v_loc != master_connector->num_v_tile - 1)
> continue;
> - crtc_state->sync_mode_slaves_mask = 0;
> - tile_group_id = connector->tile_group->id;
> - 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)
> - 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;
> - if (master_connector->tile_group->id != tile_group_id)
> - 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;
> - }
> + 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);
> }
> - drm_connector_list_iter_end(&conn_iter);
> -
> - if (!master_crtc) {
> - DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> - connector_state->crtc->base.id);
> - return -EINVAL;
> + if (master_conn_state->crtc) {
> + master_crtc = master_conn_state->crtc;
> + break;
> }
> + }
> + drm_connector_list_iter_end(&conn_iter);
>
> - 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_DEBUG_KMS("Master Transcoder = %s added for Slave CRTC = %d, slave transcoder bitmask = %d\n",
> - transcoder_name(crtc_state->master_transcoder),
> - crtc_state->uapi.crtc->base.id,
> - master_pipe_config->sync_mode_slaves_mask);
> + if (!master_crtc) {
> + DRM_DEBUG_KMS("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_DEBUG_KMS("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;
> }
>
> @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> struct drm_connector *connector;
> struct drm_connector_state *connector_state;
> int base_bpp, ret;
> - int i;
> + int i, tile_group_id = -1, num_tiled_conns = 0;
> bool retry = true;
>
> pipe_config->cpu_transcoder =
> @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
> CRTC_STEREO_DOUBLE);
>
> - /* Set the crtc_state defaults for trans_port_sync */
> - pipe_config->master_transcoder = INVALID_TRANSCODER;
> - ret = icl_add_sync_mode_crtcs(pipe_config);
> - if (ret) {
> - DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> - ret);
> - return ret;
> +
> + /* 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++;
> }
I don't see why we would need this num_tiled_conns stuff at all. Just
iterate all connectors in the state that belong to the same group.
>
> /* Pass our mode to the connectors and the CRTC to give them a chance to
> @@ -12747,6 +12775,14 @@ 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_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> + ret);
> + return ret;
> + }
> +
> encoder = to_intel_encoder(connector_state->best_encoder);
> ret = encoder->compute_config(encoder, pipe_config,
> connector_state);
> --
> 2.19.1
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list