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.syrj...@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.nav...@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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
