On Sun, Oct 06, 2019 at 08:43:31PM -0700, Manasi Navare wrote:
> On Mon, Sep 30, 2019 at 05:14:15PM +0300, Ville Syrjälä wrote:
> > On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> > > In case of tiled displays when the two tiles are sent across two CRTCs
> > > over two separate DP SST connectors, we need a mechanism to synchronize
> > > the two CRTCs and their corresponding transcoders.
> > > So use the master-slave mode where there is one master corresponding
> > > to last horizontal and vertical tile that needs to be genlocked with
> > > all other slave tiles.
> > > This patch identifies saves the master transcoder in all the slave
> > > CRTC states. This is needed to select the master CRTC/transcoder
> > > while configuring transcoder port sync for the corresponding slaves.
> > >
> > > v4:
> > > * Rebase
> > > v3:
> > > * Use master_tramscoder instead of master_crtc for valid
> > > HW state readouts (Ville)
> > > v2:
> > > * Move this to intel_mode_set_pipe_config(Jani N, Ville)
> > > * Use slave_bitmask to save associated slaves in master crtc state (Ville)
> > >
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 123 ++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_display.h | 3 +
> > > .../drm/i915/display/intel_display_types.h | 6 +
> > > 3 files changed, 132 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c05ba6af6226..4ff375d5852d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> > > return drm_atomic_crtc_needs_modeset(&state->base);
> > > }
> > >
> > > +bool
> > > +is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
> >
> > Redundant function parameter. Can be derived if needed.
>
> Ok
>
> >
> > > + const struct intel_crtc_state *state)
> >
> > 'crtc_state'
>
> Ok
>
>
> >
> > > +{
> > > + return (INTEL_GEN(dev_priv) >= 11 &&
> >
> > I don't think we need a gen check at all. The state should not have
> > master/slaves set if the feature is not supported.
>
> Ok will remove this here and for master as well
>
> >
> > > + (state->master_transcoder != INVALID_TRANSCODER ||
> > > + state->sync_mode_slaves_mask));
> > > +}
> > > +
> > > +static bool
> > > +is_trans_port_sync_master(struct drm_i915_private *dev_priv,
> > > + const struct intel_crtc_state *state)
> > > +{
> > > + return (INTEL_GEN(dev_priv) >= 11 &&
> > > + (state->master_transcoder == INVALID_TRANSCODER &&
> > > + state->sync_mode_slaves_mask));
> > > +}
> > > +
> > > /*
> > > * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> > > * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > > @@ -11773,6 +11791,91 @@ 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 drm_crtc *crtc,
> >
> > intel_ types all over please.
>
> Yes i think this patch was written we changed everything to intel_ states, will change it all
> to intel states
>
> But do we stll keep drm_atomic_state or change that to intel_atomic_state as well?
>
> >
> > Also don't need all three funciton arguments. Either just
> > crtc_state or state+crtc will do.
>
> Hmm, but I do need the atomic_state, crtc_state and also the crtc in the modeset
One can always derive the others.
Option 1:
foo(crtc_state)
{
crtc = crtc_state->crtc;
state = crtc_state->state;
...
}
Option 2:
foo(state, crtc)
{
crtc_state = get_crtc_state(state, crtc);
...
}
IMO life is more pleasant when you don't have to think about
every function needing to be passed a different set of redundant
information.
>
>
> >
> > > + struct intel_crtc_state *crtc_state,
> > > + struct drm_atomic_state *state)
> > > +{
> > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > + struct drm_connector *master_connector, *connector;
> > > + struct drm_connector_state *connector_state;
> > > + 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;
> > > +
> > > + /*
> > > + * 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.
> > > + */
> > > + for_each_new_connector_in_state(state, connector, connector_state, i) {
> > > + if (connector_state->crtc != crtc)
> > > + continue;
> > > + if (!connector->has_tile)
> > > + continue;
> > > + if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
> > > + crtc_state->base.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)
> > > + 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,
> > > + 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;
> > > + }
> > > + }
> > > + 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;
> > > + }
> > > +
> > > + master_crtc_state = drm_atomic_get_crtc_state(state,
> > > + 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->base.crtc->base.id,
> > > + master_pipe_config->sync_mode_slaves_mask);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
> > > struct drm_crtc_state *_crtc_state)
> > > {
> > > @@ -12276,6 +12379,12 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > > if (IS_G4X(dev_priv) ||
> > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > saved_state->wm = crtc_state->wm;
> > > + /* Save the slave bitmask which gets filled for master crtc state during
> > > + * slave atomic check call.
> > > + */
> >
> > Wrong comment format.
>
> What is wrong with the format?
Should look like:
/*
* foo
* bar
*/
not
/* foo
* bar
*/
Hmm. Looks like checkpatch doesn't complain about that anymore,
so maybe some people have finally come to accept either format.
>
> >
> > > + if (is_trans_port_sync_master(dev_priv, crtc_state))
> > > + saved_state->sync_mode_slaves_mask =
> > > + crtc_state->sync_mode_slaves_mask;
> > >
> > > /* Keep base drm_crtc_state intact, only clear our extended struct */
> > > BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > @@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > drm_mode_set_crtcinfo(&pipe_config->base.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(crtc, pipe_config, state);
> > > + if (ret) {
> > > + DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > +
> > > /* Pass our mode to the connectors and the CRTC to give them a chance to
> > > * adjust it according to limitations or connector properties, and also
> > > * a chance to reject the mode entirely.
> > > @@ -12882,6 +13000,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > PIPE_CONF_CHECK_INFOFRAME(hdmi);
> > > PIPE_CONF_CHECK_INFOFRAME(drm);
> > >
> > > + if (INTEL_GEN(dev_priv) >= 11) {
> >
> > Just do the check uncoditionally.
>
> Ok makes sens, will remove the gen check
>
> Regards
> Manasi
>
> >
> > > + PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
> > > + PIPE_CONF_CHECK_I(master_transcoder);
> > > + }
> > > +
> > > #undef PIPE_CONF_CHECK_X
> > > #undef PIPE_CONF_CHECK_I
> > > #undef PIPE_CONF_CHECK_BOOL
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > > index b1ae0e59c715..1623face436b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -93,6 +93,7 @@ enum pipe {
> > > #define pipe_name(p) ((p) + 'A')
> > >
> > > enum transcoder {
> > > + INVALID_TRANSCODER = -1,
> > > /*
> > > * The following transcoders have a 1:1 transcoder -> pipe mapping,
> > > * keep their values fixed: the code assumes that TRANSCODER_A=0, the
> > > @@ -453,6 +454,8 @@ enum drm_mode_status
> > > intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv,
> > > const struct drm_display_mode *mode);
> > > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> > > +bool is_trans_port_sync_mode(struct drm_i915_private *i915,
> > > + const struct intel_crtc_state *state);
> > >
> > > void intel_plane_destroy(struct drm_plane *plane);
> > > void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index d5cc4b810d9e..17ff34ca298b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -991,6 +991,12 @@ struct intel_crtc_state {
> > >
> > > /* Forward Error correction State */
> > > bool fec_enable;
> > > +
> > > + /* Pointer to master transcoder in case of tiled displays */
> > > + enum transcoder master_transcoder;
> > > +
> > > + /* Bitmask to indicate slaves attached */
> > > + u8 sync_mode_slaves_mask;
> > > };
> > >
> > > struct intel_crtc {
> > > --
> > > 2.19.1
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
