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@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) > > "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@xxxxxxxxx> > > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
