[Intel-gfx] [PATCH v2 1/4] drm/i915/icl: Assign Master slave crtc links for Transcoder Port Sync
Manasi Navare
manasi.d.navare at intel.com
Fri Jun 21 18:40:20 UTC 2019
As per the feedback, I tested the code with saving master_transcoder instead of master_crtc
and in the HW state readout I loop through all transcoders to fill the trans_port_sync_slaves bitmask.
This is working great and i dont see any pipe state mismatches now.
Thanks for all the feedback and I will be sending the new patch revision out later today.
Regards
Manasi
On Thu, Jun 13, 2019 at 01:13:28PM -0700, Manasi Navare wrote:
> On Thu, Jun 13, 2019 at 08:12:37PM +0300, Ville Syrjälä wrote:
> > On Thu, Jun 13, 2019 at 09:41:42AM -0700, Manasi Navare wrote:
> > > On Thu, Jun 13, 2019 at 12:10:57PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> > > > > On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > > > > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > > > > > 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 CRTC pointer in all the slave
> > > > > > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > > > > > >
> > > > > > > > > > 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 at ffwll.ch>
> > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > > > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > > > > > > drivers/gpu/drm/i915/intel_drv.h | 6 ++
> > > > > > > > > > 2 files changed, 95 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > index b276345779e6..92dea2231499 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > > > > > > return 0;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +static int icl_add_genlock_crtcs(struct drm_crtc *crtc,
> > > > > > > > > > + 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;
> > > > > > > > > > + 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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > > > > > + connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > > > > > + continue;
> > > > > > > > > > + crtc_state->master_crtc = NULL;
> > > > > > > > > > + 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 add 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);
> > > > > > > > > > +
> > > > > > > > > > + crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > > > > > + to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > > > > > + BIT(to_intel_crtc(crtc)->pipe);
> > > > > > > > > > + DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > > > > > + master_crtc->base.id,
> > > > > > > > > > + crtc_state->base.crtc->base.id,
> > > > > > > > > > + to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > > > > > > struct drm_crtc_state *crtc_state)
> > > > > > > > > > {
> > > > > > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > > > > + saved_state->trans_port_sync_slaves =
> > > > > > > > > > + crtc_state->trans_port_sync_slaves;
> > > > > > > > > >
> > > > > > > > > > /* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > > > > > > BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > > > > > > drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > > > > > > CRTC_STEREO_DOUBLE);
> > > > > > > > > >
> > > > > > > > > > + ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > > > > > + if (ret) {
> > > > > > > > > > + DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > > > > > + 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.
> > > > > > > > >
> > > > > > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > > > > >
> > > > > > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > > > > >
> > > > > > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > > > > >
> > > > > > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > > > > > cases other than we might want to handle tiled displays that way one
> > > > > > > > day to make life easier for userspace. But that has other issues such
> > > > > > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > > > > > of the tiles.
> > > > > > > >
> > > > > > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > > > > > no point in trying to shoehorn both things into the same bucket.
> > > > > > >
> > > > > > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > > > > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > > > > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > > > > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > > > > >
> > > > > > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > > > > > do I identify which crtc is slave?
> > > > > > > Currently I check for !master_crtc and that tells me that it is not slave.
> > > > > >
> > > > > > INVALID_TRANSCODDER or something?
> > > > >
> > > > > Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> > > > > can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> > > > > in slave crtc_state, wha do you think?
> > > >
> > > > Only if you ignore the fact that transcoder EDP can be the master.
> > >
> > > Actually Transcoder EDP can never be the master so there should be a warning if TRANSCODER EDP gets set as a master in teh configuration time itself.
> >
> > IIRC the docs say it can be master, but not slave.
>
> So its probably better to just store the master cpu transcoder directly in the slave crtc state. In that case, I will have to add TRANSCODER_INVALID = -1
> to the enum transcoder just like we do for the PIPE_INVALID and then I can use TRANSCODER_INVALID to decide whether its a slave or master.
>
> Also for trans_port_sync_slaves bitmask, in the HW state readout for master CRTC, I will loop through all the active transcoders and find the pipe for the
> transcoders that have port sync enable set in their TRANS_PORT_SYNC reg
>
> Does this sound like a correct approach?
>
> Manasi
>
> >
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list