On Fri, Dec 13, 2019 at 10:28:49PM +0200, Ville Syrjälä wrote: > On Wed, Dec 11, 2019 at 01:14:24PM -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. > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 7263eaa66cda..c0a2dab3fe67 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -12048,6 +12048,12 @@ 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 void initialize_trans_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_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state) > > { > > struct drm_crtc *crtc = crtc_state->uapi.crtc; > > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state) > > 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; > > + int i, tile_group_id = 0, num_tiled_conns = 0; > > > > if (INTEL_GEN(dev_priv) < 11) > > return 0; > > > > + /* If all tiles not present do not make master slave assignments > > + * Here we assume all tiles belong to the same tile group for now. > > + */ > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) { > > + if (connector->has_tile) { > > + if (!tile_group_id) > > + tile_group_id = connector->tile_group->id; > > Isn't 0 a valid tile group id? > > > + num_tiled_conns++; > > + } > > This whole thing looks confused. Should it not just look for the same > tile group as what the current connector belongs to? > > > + } > > + > > /* > > * 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 > > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state) > > if (!connector->has_tile) > > continue; > > if (crtc_state->hw.mode.hdisplay != connector->tile_h_size || > > - crtc_state->hw.mode.vdisplay != connector->tile_v_size) > > + crtc_state->hw.mode.vdisplay != connector->tile_v_size) { > > + initialize_trans_port_sync_mode_state(crtc_state); > > return 0; > > + } > > + if (connector->tile_group->id == tile_group_id && > > + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) { > > + initialize_trans_port_sync_mode_state(crtc_state); > > + return 0; > > + } > > if (connector->tile_h_loc == connector->num_h_tile - 1 && > > connector->tile_v_loc == connector->num_v_tile - 1) > > continue; > > This whole thing seems kinda overly complicated. I suggest it should > just blindly go through all connectors of the same tile group and pick > the lowest transcoder as the master, which is the logic Jose is using > for MST. Except I guess we have to special case the EDP transcoder > for port sync since it can't be a slave. So a simple numeric comparison > won't quite do like used for MST. Hmm. Except that won't actually work since .cpu_transcoder won't have been populated yet for the later pipes. So we can't use that in .compute_config(). So either we do this after .compute_config() is done for everyone or we just calculate the cpu_transcoder on the spot (I mean it's just a trivial port==A->EDP else->pipe so no big deal). > > And then we should probably move this thing to the encoder > .compute_config(). I suppose it should look in the end something like: > > compute_config() { > ... > crtc_state->master = compute_master_transcoder(); > crtc_state->slaves = 0; > if (master_transcoder == cpu_transcoder) > crtc_state->master = INVALID; > crtc_state->slave = compute_slave_transcoders(); > } > } > > That keeps it very readable and avoids the confusing stuff of > comptue_config() for one pipe randomly mutating the states of > the other pipes. > > -- > Ville Syrjälä > Intel -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
