| Message ID | 20191211211425.17821-3-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915 fixes to handle hotplug cases on 8K tiled monitor | expand |
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@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > 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) "reset" rather than "initialize" might be more consistent with similar things elsewhere in the driver? > +{ > + 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 This comment seems like it should go farther down the function; this block isn't directly responsible for master/slave assignments, so it's a bit confusing here. Also, you might want to clarify what "present" means in this case. E.g., explain that since you've already added all tiles of the same monitor to the transaction (earlier in intel_atomic_check), then if the number of tiles in the tile group is smaller than the size of the tile group it means that at least one of the tiles isn't active. > + * Here we assume all tiles belong to the same tile group for now. Should this sentence be a FIXME:? If we plug in 2x 2-tile monitors on TGL, this would be problematic, right? The logic changes seem correct (if we assume that only a single tiled monitor is in use), so Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > + */ > + 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; > + num_tiled_conns++; > + } > + } > + > /* > * 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; > -- > 2.19.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 12, 2019 at 05:03:07PM -0800, Matt Roper 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@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > 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) > > "reset" rather than "initialize" might be more consistent with similar > things elsewhere in the driver? Yes I was considering other names like default/reset/initialize, but yes I think reset sounds better, will change that > > > +{ > > + 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 > > This comment seems like it should go farther down the function; this > block isn't directly responsible for master/slave assignments, so it's a > bit confusing here. Also, you might want to clarify what "present" > means in this case. E.g., explain that since you've already added all > tiles of the same monitor to the transaction (earlier in > intel_atomic_check), then if the number of tiles in the tile group is > smaller than the size of the tile group it means that at least one of > the tiles isn't active. > Yes will move this comment down > > + * Here we assume all tiles belong to the same tile group for now. Will add a FIXME I will make these changes and add your r-b, thanks Matt Manasi > > Should this sentence be a FIXME:? If we plug in 2x 2-tile monitors on > TGL, this would be problematic, right? > > The logic changes seem correct (if we assume that only a single tiled > monitor is in use), so > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > > > + */ > > + 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; > > + num_tiled_conns++; > > + } > > + } > > + > > /* > > * 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; > > -- > > 2.19.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795
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@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > 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. 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.
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@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > 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
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@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > 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? I was making sure that we dont count the connectors that dont belong to the same tile grp id. But yes I agree that the tile grp id can be assigned by current connector's tile grp id and add num_conns only if they are of the same tile grp id. > > > + } > > + > > /* > > * 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. > > 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. But the logic that we are using is always make the connector corresponding to the last tile the master and all other connectors as slaves since only 1 master (last tile) and multiple slaves. This was agreed upon with discussions with you and Danvet and Maarten almost 6 months back and the entire enable/disable sequence is based on this. I would rather keep the logic same for now since the master slave asisgnments which was designed based on consensus during the initial patches and since its working well now. The only thing that I wanted to add in this patch is reset the master_trans and slave_bitmask properly if all tiles not present in order to handle the hotplug case correctly. I will make changes for the tile grp id that you suggested for now and we can simplify the logic later as enhncements/optimizations. Sounds good? Manasi > > -- > Ville Syrjälä > Intel
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; + num_tiled_conns++; + } + } + /* * 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;
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@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)